Skip to content

feat: Add "space-variants" feature#110

Merged
spex66 merged 4 commits intomainfrom
feat/space-variants
Apr 1, 2025
Merged

feat: Add "space-variants" feature#110
spex66 merged 4 commits intomainfrom
feat/space-variants

Conversation

@spex66
Copy link
Collaborator

@spex66 spex66 commented Mar 31, 2025

  • new version 3.5.0
  • update to latest cdf-sdk v7
    • small changes how to create Group / Capabilties and spaces.apply
  • adding new feature to node level to define list of space-variants
    • space-variants: ["i", "m"]
  • suffix added to the very end (after -spc) like for rawdb-variants
  • refactoring needed to not only use node_name: str as parameter but node: NamespaceNode
  • difference to rawdb-variants is, that it is not globally but locally configured
  • might mitigate the pain until "nested space access-control" is available

technical-debt / design-flaw

  • space-variants are good to create multiple spaces easily, but they fail if access-control is required on a granular level.
  1. -spc-m typically representing data-models
  2. -spc-i typically data-instances

with the approach, it is not possible to separate read/write access between the two spaces.

That's a technical debt, which needs to be resolved with switching to cdf-tk approach, or once we can switch to "nested spaces access-control"

  • for now the approach provides convenience (speed&simplicity) for increasing demand for spaces to get work done

- new version 3.5.0
- update to latest cdf-sdk v7
- adding new feature to node level to define list of space-variants
- suffix at the very end (after `-spc`)
- refactoring needed to not only use `node_name: str` as parameter but `node: NamespaceNode`
- difference to rawdb-variants is, that it is not globally but locally configured
- might mitigate the pain until "nested space access-control" is available
@spex66 spex66 requested a review from a team as a code owner March 31, 2025 19:23
- cleanup `Optional[]` where there is a default-value
- cleanup test print from pytest case
@spex66 spex66 requested a review from a team April 1, 2025 08:45
> .. it uses a deprecated version of `actions/cache: v3.3.2`. Please update your workflow to use v3/v4 of actions/cache to avoid interruptions
@psalaberria002 psalaberria002 added the risk-review-ongoing Risk review is in progress label Apr 1, 2025
@psalaberria002 psalaberria002 self-assigned this Apr 1, 2025
@spex66 spex66 changed the title feat(space-variants) Add "space-variants" feature Apr 1, 2025
@spex66 spex66 changed the title Add "space-variants" feature feat: Add "space-variants" feature Apr 1, 2025
Comment on lines 50 to 51
# item = convert_time_attributes_to_datetime(self.dump())
# return json.dumps(item, default=utils._auxiliary.json_dump_default, indent=4)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need to keep these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Comment on lines +203 to +211
with_raw_capability: bool = True
with_datamodel_capability: bool = True
with_undocumented_capabilities: bool = False
group_prefix: str = "cdf"
aggregated_level_name: str = "allprojects"
dataset_suffix: str = "dataset"
space_suffix: str = "space"
rawdb_suffix: str = "rawdb"
rawdb_additional_variants: list[str] = ["state"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. Is it a safe change? Will clients using your library break?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is fully backward compatible, but from 3.5.0 onward supports a new (additional) config

semantic versioning from 3.4.1 to 3.5.0 is indicating that, and release-notes will too

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, something that was optional before is now mandatory. I see that as breaking. Did I misunderstand how it works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah you meant removal of Optional[..], that was only done where the default value was not None anyway.

A corner case could be that a yaml-config was configured with null which had before passed a pydantic value-validation

as this was never documented (and makes no sense) it is theoretical

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as all have default values it is still optional when parsed by pydantic, just not support None anymore

Copy link

@psalaberria002 psalaberria002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I managed to review the PR since it isn't that large, but going forward I don't think we should approve such PRs. See https://cognitedata.slack.com/archives/C0425G1JP2A/p1743509263432649

Once the comments I left in this PR are resolved ping me again for a final risk review.

Copy link

@psalaberria002 psalaberria002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦄

Copy link
Collaborator Author

@spex66 spex66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

answered or resolved

Comment on lines +203 to +211
with_raw_capability: bool = True
with_datamodel_capability: bool = True
with_undocumented_capabilities: bool = False
group_prefix: str = "cdf"
aggregated_level_name: str = "allprojects"
dataset_suffix: str = "dataset"
space_suffix: str = "space"
rawdb_suffix: str = "rawdb"
rawdb_additional_variants: list[str] = ["state"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah you meant removal of Optional[..], that was only done where the default value was not None anyway.

A corner case could be that a yaml-config was configured with null which had before passed a pydantic value-validation

as this was never documented (and makes no sense) it is theoretical

Comment on lines +203 to +211
with_raw_capability: bool = True
with_datamodel_capability: bool = True
with_undocumented_capabilities: bool = False
group_prefix: str = "cdf"
aggregated_level_name: str = "allprojects"
dataset_suffix: str = "dataset"
space_suffix: str = "space"
rawdb_suffix: str = "rawdb"
rawdb_additional_variants: list[str] = ["state"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as all have default values it is still optional when parsed by pydantic, just not support None anymore

Comment on lines 50 to 51
# item = convert_time_attributes_to_datetime(self.dump())
# return json.dumps(item, default=utils._auxiliary.json_dump_default, indent=4)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@spex66 spex66 merged commit 50fde95 into main Apr 1, 2025
3 checks passed
@spex66 spex66 deleted the feat/space-variants branch April 1, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-review-ongoing Risk review is in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants