feat: IHS-190 (part of IFC-2162) Add namespace restriction parameter to generic schemas#807
Conversation
…ed_namespaces but also missing fields IHS-190
…rough infrahubctl command layer. Infrahub API is mocked IHS-190
…nvalid namespace is loaded from SDK methods. Infrahub API is mocked IHS-190
polmichel
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## infrahub-develop #807 +/- ##
====================================================
+ Coverage 80.33% 81.04% +0.70%
====================================================
Files 115 121 +6
Lines 9865 10644 +779
Branches 1504 1583 +79
====================================================
+ Hits 7925 8626 +701
- Misses 1419 1501 +82
+ Partials 521 517 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 35 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Small comment on this PR. It should target the |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds two optional fields to 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@infrahub_sdk/schema/main.py`:
- Around line 292-294: GenericSchema is missing the hierarchical and
restricted_namespaces attributes present in GenericSchemaAPI, causing those
values (e.g., restricted_namespaces from valid_generic_schema.json) to be
dropped when parsed via SchemaRoot; add the two fields to GenericSchema by
declaring hierarchical: bool | None = None and restricted_namespaces: list[str]
| None = None on the GenericSchema dataclass/model so it matches
GenericSchemaAPI and preserves round-trip parsing of those properties.
🧹 Nitpick comments (2)
tests/unit/sdk/test_schema.py (2)
516-537: Variableerror_messageis shadowed on Line 536.
error_messageis first defined on Line 516 as the mock response string, then reassigned on Line 536 to the extracted value from the response. This shadowing is confusing — use a distinct name for one of them.Suggested fix
- error_message = response.errors["errors"][0]["message"] - assert re.search(r"(?s)restricted namespaces(?=.*Dog)(?=.*Cat)", error_message) + actual_error = response.errors["errors"][0]["message"] + assert re.search(r"(?s)restricted namespaces(?=.*Dog)(?=.*Cat)", actual_error)
488-538: Test only covers the async path; consider adding sync coverage.Existing tests in this file consistently parametrize over
["standard", "sync"]client types (e.g., Lines 47, 66, 93). This test only exercisesInfrahubClient(async). Consider adding a sync variant usingInfrahubClientSync.schema.loadfor parity, consistent with the existing pattern in this file. As per coding guidelines,infrahub_sdk/**/*.py: Follow async/sync dual pattern for new features in the Python SDK.
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
LGTM, but as indicated by the comment I'm not sure about that second test.
| assert InfrahubSchemaBase._get_schema_name(schema="BuiltinIPAddress") == "BuiltinIPAddress" | ||
|
|
||
|
|
||
| async def test_schema_load_rejected_when_node_namespace_violates_generic_restricted_namespaces( |
There was a problem hiding this comment.
I'm not sure that this test actually tests anything meaningful, since we're just mocking the response. I.e. are we doing something here that isn't already covered by other tests?
There was a problem hiding this comment.
This tests that the error message is going from the API response to the user through the SDK layer. This does not bring much value, I could understand if you would rather delete it.
There was a problem hiding this comment.
I think in this case that the test name is a bit confusing as it really doesn't have anything to do with generics and restricted names but only how the SDK responds to errors in general. It's just that the dummy data happens to be one of this new kind, does that make sense?
There was a problem hiding this comment.
Yes, I generalized and simplified the unit test.
Deploying infrahub-sdk-python with
|
| Latest commit: |
79816b6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2e1aab34.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pmi-20260204-namespace-restr.infrahub-sdk-python.pages.dev |
3 participants
To understand the global feature, see this PR: opsmill/infrahub#8319
This PR is about updating
infrahub_sdk.schema.main.GenericSchemaAPIwith the new generic schema parameter "restricted_namespaces".Goal
See opsmill/infrahub#8319
Non-goals
Added another missing attribute to
infrahub_sdk.schema.main.GenericSchemaAPI: hierarchical. This attribute was already into the internal Infrahub generic schema model.What changed
Generic schemas into SDK API.
How-to review
This PR is quite small: commit by commit, in the chronological order.
How to test
Automated back-end tests:
uv run pytest tests/unit/ctl/test_schema_app.py tests/unit/sdk/test_schema.pyApplication testing scenarios:
Impact & rollout
Deployment notes: safe to deploy
Closes IHS-190
Summary by CodeRabbit
New Features
Tests