-
Notifications
You must be signed in to change notification settings - Fork 6
Process schema deprecation warnings #582
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
WalkthroughThis pull request adds support for deprecation warnings in schema loading and checking operations. New warning-related models (SchemaWarningType, SchemaWarningKind, and SchemaWarning) are introduced to represent structured warning information. The SchemaLoadResponse model is updated to include a warnings field populated from server responses. The CLI schema commands are enhanced with a helper function to display warnings to users, and the load() and check() operations are modified to extract and render warnings alongside their existing functionality. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: |
7b7612d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5584e1e4.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-process-schema-deprecati.infrahub-sdk-python.pages.dev |
73b5f9b to
11deff7
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #582 +/- ##
====================================================
- Coverage 75.27% 75.25% -0.03%
====================================================
Files 108 108
Lines 9375 9398 +23
Branches 1864 1868 +4
====================================================
+ Hits 7057 7072 +15
- Misses 1823 1830 +7
- Partials 495 496 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
11deff7 to
d687229
Compare
d687229 to
7b7612d
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/ctl/schema.py (1)
203-207: Useconsole.print()for consistency.Lines 204 and 206 use the built-in
print()function, while the rest of the file consistently usesconsole.print()from Rich. This inconsistency could lead to formatting differences in output.Apply this diff:
if response == {"diff": {"added": {}, "changed": {}, "removed": {}}}: - print("No diff") + console.print("No diff") else: - print(yaml.safe_dump(data=response, indent=4)) + console.print(yaml.safe_dump(data=response, indent=4))infrahub_sdk/schema/__init__.py (1)
93-111: LGTM!The warning model hierarchy is well-designed with proper Pydantic validation, clear field descriptions, and a useful
displayproperty for formatting. The structure is extensible for future warning types beyond deprecation.Consider adding the new public models (
SchemaWarningType,SchemaWarningKind,SchemaWarning) to the__all__list (lines 51-67) for better API discoverability and consistency with other exported models.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelog/+f6791a3d.added.md(1 hunks)infrahub_sdk/ctl/schema.py(3 hunks)infrahub_sdk/schema/__init__.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/schema/__init__.pyinfrahub_sdk/ctl/schema.py
infrahub_sdk/ctl/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/ctl/**/*.py: Build CLI commands with Typer
Organize and keep all CLI commands within infrahub_sdk/ctl/
Files:
infrahub_sdk/ctl/schema.py
🧬 Code graph analysis (1)
infrahub_sdk/ctl/schema.py (1)
infrahub_sdk/schema/__init__.py (2)
SchemaWarning(107-110)display(102-104)
🔇 Additional comments (8)
changelog/+f6791a3d.added.md (1)
1-1: LGTM!The changelog entry clearly documents the new deprecation warnings feature.
infrahub_sdk/ctl/schema.py (5)
17-17: LGTM!The import correctly brings in the new
SchemaWarningmodel for warning display functionality.
156-157: LGTM!The warning display is correctly integrated after schema loading, leveraging the validated warnings from
SchemaLoadResponse.
193-196: LGTM!The defensive check with early return improves error handling when the check operation fails or returns no response.
197-202: LGTM!The enhanced user feedback with per-schema validation messages and warning extraction improves the check command's output clarity.
Note: The manual validation at lines 200-202 using
SchemaWarning.model_validate()could raise aValidationErrorif the server returns malformed warning data. Consider whether wrapping this in a try-except would provide a better user experience, or if failing loudly is the preferred behavior for detecting server issues.
209-213: LGTM!The helper function centralizes warning display logic with clear formatting. It correctly leverages the
displayproperty fromSchemaWarningKindto format the kinds list.infrahub_sdk/schema/__init__.py (2)
192-194: LGTM!The defensive extraction pattern
status.get("warnings") or []safely handles missing or null warning data from the server, with Pydantic validating the structure automatically.
827-827: LGTM!The warnings field is properly defined with Pydantic best practices, using
default_factory=listfor the mutable default and providing clear documentation.
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.
early returns, small functions, very nice 👍
Displays schema deprecation warnings for
infrahubctlwhen loading or checking schemas. Uses the newwarningskey from opsmill/infrahub#7424.Summary by CodeRabbit