feat(infield): add validation for new DataExplorationConfig properties#3009
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
…es-and-notifications-configurations
…fications-configurations' of https://github.com/cognitedata/toolkit into FO-3617-add-toolkit-validations-for-activities-and-notifications-configurations
…-3617-add-toolkit-validations-for-activities-and-notifications-configurations
…es-and-notifications-configurations
☂️ Code Coverage
Overall Coverage
New Files
Modified Files
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3009 +/- ##
==========================================
+ Coverage 85.91% 85.93% +0.01%
==========================================
Files 469 470 +1
Lines 43059 43147 +88
==========================================
+ Hits 36994 37078 +84
- Misses 6065 6069 +4
🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the InFieldCDMViewPropertiesRuleSet to validate that InField CDM card views exist in CDF and contain all required properties. It also updates dependency tracking in InFieldCDMLocationConfigIO and adds the necessary fields to the YAML configuration models. Feedback includes expanding dependency tracking to include asset_properties_card, avoiding broad exception handling in the validation logic, and batching API calls to prevent performance issues during view retrieval.
gulkaktus
left a comment
gulkaktus
left a comment
There was a problem hiding this comment.
Left a few comments, looks good mostly!
| def get_dependencies(cls, resource: InFieldCDMLocationConfigYAML) -> Iterable[tuple[type[ResourceIO], Identifier]]: | ||
| if data_exploration_config := resource.data_exploration_config: | ||
| for card_attr in INFIELD_CDM_CARD_VIEW_ATTR_TO_JSON_KEY: | ||
| if card_mapping := getattr(data_exploration_config, card_attr, None): | ||
| yield ( | ||
| ViewIO, | ||
| ViewId( | ||
| space=card_mapping.space, | ||
| external_id=card_mapping.external_id, | ||
| version=card_mapping.version, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
| def get_dependencies(cls, resource: InFieldCDMLocationConfigYAML) -> Iterable[tuple[type[ResourceIO], Identifier]]: | |
| if data_exploration_config := resource.data_exploration_config: | |
| for card_attr in INFIELD_CDM_CARD_VIEW_ATTR_TO_JSON_KEY: | |
| if card_mapping := getattr(data_exploration_config, card_attr, None): | |
| yield ( | |
| ViewIO, | |
| ViewId( | |
| space=card_mapping.space, | |
| external_id=card_mapping.external_id, | |
| version=card_mapping.version, | |
| ), | |
| ) | |
| def get_dependencies(cls, resource: InFieldCDMLocationConfigYAML) -> Iterable[tuple[type[ResourceIO], Identifier]]: | |
| if resource.data_exploration_config is None: | |
| return | |
| for card_attr in INFIELD_CDM_CARD_VIEW_ATTR_TO_JSON_KEY: | |
| card_mapping = getattr(resource.data_exploration_config, card_attr, None) | |
| if card_mapping is None: | |
| continue | |
| yield ( | |
| ViewIO, | |
| ViewId( | |
| space=card_mapping.space, | |
| external_id=card_mapping.external_id, | |
| version=card_mapping.version, | |
| ), | |
| ) |
Nitpick, but it would be nice to reduce the indent here if we can, for example something like this
|
|
||
|
|
||
| class InFieldCDMViewPropertiesRuleSet(ToolkitGlobalRuleSet): | ||
| CODE: ClassVar[str] = "INFIELD-CDM-VIEW-PROPERTIES" |
There was a problem hiding this comment.
| CODE: ClassVar[str] = "INFIELD-CDM-VIEW-PROPERTIES" | |
| CODE_PREFIX: ClassVar[str] = "INFIELD-CDM" |
This should be a prefix to align with the parent class ToolkitGlobalRuleSet
| view = views_by_id.get(view_id) | ||
| if view is None: | ||
| yield ConsistencyError( | ||
| code=self.CODE, |
There was a problem hiding this comment.
Continuing from the comment above, this should be structured like CODE_PREFIX + "-Something"
You can find an example in cognite_toolkit/_cdf_tk/rules/_dependencies.py or cognite_toolkit/_cdf_tk/rules/_functions.py.
For example this one could be something like f"{CODE_PREFIX}-VIEW-NOT-FOUND"
| missing = required - set(view.properties.keys()) | ||
| if missing: | ||
| yield ConsistencyError( | ||
| code=self.CODE, |
There was a problem hiding this comment.
For example this one could be something like f"{CODE_PREFIX}-VIEW-MISSING_PROPERTIES"
…es-and-notifications-configurations
Magssch
left a comment
Magssch
left a comment
There was a problem hiding this comment.
Looks good, thank you for the contribution @serhiikindrat-cdf ! :)
serhiikindrat-cdf
deleted the
FO-3617-add-toolkit-validations-for-activities-and-notifications-configurations
branch
3 participants
Description
Add build-time validation for InField CDM location configs that reference activities and notifications card views in dataExplorationConfig.
Test check for the view existance:
Test check for the required properties existance:
Tasks:
Bump
Changelog
Improved
cdf buildwithInFieldCDMLocationConfigresources, Toolkit now more strictly validatesassetActivitiesCardandassetNotificationsCardview references and their required properties.