-
Notifications
You must be signed in to change notification settings - Fork 6
Add to object conversion input #558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughChanges in infrahub_sdk/convert_object_type.py: the error message in Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying infrahub-sdk-python with
|
| Latest commit: |
5498204
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f222ad5a.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://lgu-obj-conv-use-default-val.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #558 +/- ##
===========================================
+ Coverage 75.73% 75.76% +0.02%
===========================================
Files 101 101
Lines 8969 8969
Branches 1769 1768 -1
===========================================
+ Hits 6793 6795 +2
+ Misses 1689 1688 -1
+ Partials 487 486 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
infrahub_sdk/convert_object_type.py (2)
53-59: Prefer Optional[bool] foruse_default_valueto avoid serializing False by defaultModel dumps that only exclude None will otherwise include
"use_default_value": Falseunnecessarily. Making it Optional avoids noisy payloads and aligns with the “exactly one set” pattern used by other fields.Apply this diff:
- use_default_value: bool = False + use_default_value: bool | None = None @@ - fields_set = [self.source_field is not None, self.data is not None, self.use_default_value is True] + fields_set = [self.source_field is not None, self.data is not None, self.use_default_value is True]Note: No change needed in the validator logic;
is Truestill works as intended.
57-60: Guard against empty/whitespace-onlysource_fieldTreating
""as “set” will pass validation but is likely invalid upstream. Reject early.Apply this diff:
@model_validator(mode="after") def check_only_one_field(self) -> ConversionFieldInput: + if self.source_field is not None and not self.source_field.strip(): + raise ValueError("`source_field`, if provided, must be a non-empty string") fields_set = [self.source_field is not None, self.data is not None, self.use_default_value is True] if sum(fields_set) != 1: raise ValueError("Exactly one of `source_field`, `data` or `use_default_value` must be set") return self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrahub_sdk/convert_object_type.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/convert_object_type.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (3.13)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
infrahub_sdk/convert_object_type.py (2)
39-40: Error message clarity: LGTMBackticks improve readability/consistency across validation errors.
53-59: Confirm backend schema supportsuse_default_valuePlease verify the GraphQL mutation/validation layer accepts the new
use_default_valuekey infields_mappingto prevent server-side rejections.
infrahub_sdk/convert_object_type.py
Outdated
| Use `source_field` to reuse the value of the corresponding field of the object being converted. | ||
| Use `data` to specify the new value for the field. | ||
| Use `use_default_value` to set the destination field to its schema default. | ||
| Only one of `source_field` or `data` can be specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Only one of `source_field` or `data` can be specified. | |
| Only one of `source_field`, `data`, or `use_default_value` can be specified. |
17fee47 to
5498204
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
infrahub_sdk/convert_object_type.py (2)
58-60: Harden validation: treat empty/whitespacesource_fieldas “not set”An empty or whitespace-only string shouldn’t count as “set”.
Apply this diff within the validator:
- fields_set = [self.source_field is not None, self.data is not None, self.use_default_value is True] + fields_set = [bool(self.source_field and self.source_field.strip()), self.data is not None, self.use_default_value is True]Optionally, enforce non-empty when provided:
# outside selected lines from pydantic import BaseModel, model_validator, Field # add Field # and update the field definition: source_field: str | None = Field(default=None, min_length=1)
54-54: Prefer Optional[bool] for use_default_value
Change- use_default_value: bool = False + use_default_value: bool | None = Noneto align with other optional fields and prevent default False from appearing in serialized payloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrahub_sdk/convert_object_type.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/convert_object_type.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.11)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.9)
- GitHub Check: unit-tests (3.12)
🔇 Additional comments (2)
infrahub_sdk/convert_object_type.py (2)
39-39: Clearer error message formatting — LGTMBackticks improve readability and consistency across validation errors.
48-50: Docstring now documentsuse_default_valueand mutual exclusivity — niceThis addresses the earlier feedback to include the new option and the “only one of” rule.
Add
use_default_valuein order to specify when we want of a field to use the default value instead of potentially reusing the value of an existing matching input field.Summary by CodeRabbit
New Features
Bug Fixes
Style