Merge develop into infrahub-develop#856
Merged
polmichel merged 12 commits intoinfrahub-developfrom Feb 27, 2026
Merged
Conversation
…hat will be executed by the CI.
…i command since they are independent checks
New /pre-ci & /feedback commands for Claude
…850) * IHS-156 fix SDK from_pool attribute management before querying GraphQL API * IHS-156 refactor the test cases to have a better view on the tests perimeter * towncrier regarding Github issue #497 * IHS-156 update AGENTS doc using the command /feedback * IHS-156 is_from_pool_attribute typing and naming * IHS-156 refactor generate_input_data * IHS-156 remove Jira issue related comment in the tests * IHS-156 removed a part of fixtures to upper level * IHS-156 tested all cases of generated_input_data * IHS-156 fixed a bug in test_relationship_from_pool.py * IHS-156 side effect bugs regarding from_pool attributes * IHS-156 last feedbacks regarding documentation and variable naming * IHS-156 fix typing error * IHS-156 renamed payload_dict -> payload
…-develop-into-infrahub-develop # Conflicts: # tests/unit/sdk/conftest.py
github-actions
bot
added
the
type/documentation
WalkthroughRefactors SDK attribute GraphQL payload handling: adds 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Deploying infrahub-sdk-python with
|
| Latest commit: |
52d475e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://667cdbf9.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pmi-20260226-merge-develop-i.infrahub-sdk-python.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/sdk/pool/test_attribute_from_pool.py (1)
133-204: Consider adding sync client tests for node-based from_pool.The node-based
from_pooltests (lines 140-204) only cover the async client. For parity with the dict-based tests that cover both async and sync paths, consider adding corresponding sync client tests fortest_attribute_with_pool_node_generates_from_poolandtest_attribute_with_pool_node_generates_mutation_query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/sdk/pool/test_attribute_from_pool.py` around lines 133 - 204, Tests only exercise async client paths for node-based from_pool; add mirror sync-client tests to ensure parity. Duplicate the two async test cases test_attribute_with_pool_node_generates_from_pool and test_attribute_with_pool_node_generates_mutation_query but construct InfrahubNode instances using the sync client fixture (e.g., a sync client name used elsewhere in dict-based tests), call the sync equivalents of _generate_input_data and _generate_mutation_query on the nodes, and assert the same expectations (vlan_id from_pool id and mutation object value None). Use the same helper symbols (InfrahubNode, vlan_schema, ipaddress_pool_schema, ipam_ipprefix_schema, ipam_ipprefix_data, NODE_POOL_ID) so the new tests mirror the async ones exactly but target the sync code paths.tests/unit/sdk/test_attribute_generate_input_data.py (1)
360-404:_FakeNodeis missingget_node_metadatamethod fromCoreNodeBaseprotocol.Per
infrahub_sdk/protocols_base.pylines 175-206, theCoreNodeBaseprotocol requires aget_node_metadata(self) -> NodeMetadata | Nonemethod. While the current tests pass because the tested code paths don't invoke this method, adding it would ensure full protocol compliance and prevent potential issues if future code changes require it.♻️ Add missing protocol method
def get_raw_graphql_data(self) -> dict | None: return None + + def get_node_metadata(self) -> None: + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/sdk/test_attribute_generate_input_data.py` around lines 360 - 404, The _FakeNode test stub lacks the protocol-required method get_node_metadata; add a method def get_node_metadata(self) -> NodeMetadata | None: that returns None (or a minimal NodeMetadata instance if tests need it) to satisfy the CoreNodeBase protocol; update the _FakeNode class in the test file (referencing _FakeNode and CoreNodeBase) and import or reference NodeMetadata as needed so the signature type is correct.infrahub_sdk/node/attribute.py (1)
139-158: Minor docstring inconsistency.The docstring mentions "Returns a ResolvedValue object" but the actual return type is
_GraphQLPayloadAttribute. Consider updating for accuracy.📝 Fix docstring
def _generate_input_data(self) -> _GraphQLPayloadAttribute: """Build the input payload for a GraphQL mutation on this attribute. - Returns a ResolvedValue object, which contains all the data required. + Returns a _GraphQLPayloadAttribute object, which contains all the data required. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/node/attribute.py` around lines 139 - 158, Update the docstring for the method _generate_input_data to accurately state the return type and purpose: replace the incorrect "Returns a ResolvedValue object" text with a description that it returns a _GraphQLPayloadAttribute (or the appropriate GraphQL payload type) and optionally mention it builds the input payload for a GraphQL mutation; ensure the one-line summary and the Returns section reference _GraphQLPayloadAttribute and keep the existing brief description of the method's behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@infrahub_sdk/node/attribute.py`:
- Around line 139-158: Update the docstring for the method _generate_input_data
to accurately state the return type and purpose: replace the incorrect "Returns
a ResolvedValue object" text with a description that it returns a
_GraphQLPayloadAttribute (or the appropriate GraphQL payload type) and
optionally mention it builds the input payload for a GraphQL mutation; ensure
the one-line summary and the Returns section reference _GraphQLPayloadAttribute
and keep the existing brief description of the method's behavior.
In `@tests/unit/sdk/pool/test_attribute_from_pool.py`:
- Around line 133-204: Tests only exercise async client paths for node-based
from_pool; add mirror sync-client tests to ensure parity. Duplicate the two
async test cases test_attribute_with_pool_node_generates_from_pool and
test_attribute_with_pool_node_generates_mutation_query but construct
InfrahubNode instances using the sync client fixture (e.g., a sync client name
used elsewhere in dict-based tests), call the sync equivalents of
_generate_input_data and _generate_mutation_query on the nodes, and assert the
same expectations (vlan_id from_pool id and mutation object value None). Use the
same helper symbols (InfrahubNode, vlan_schema, ipaddress_pool_schema,
ipam_ipprefix_schema, ipam_ipprefix_data, NODE_POOL_ID) so the new tests mirror
the async ones exactly but target the sync code paths.
In `@tests/unit/sdk/test_attribute_generate_input_data.py`:
- Around line 360-404: The _FakeNode test stub lacks the protocol-required
method get_node_metadata; add a method def get_node_metadata(self) ->
NodeMetadata | None: that returns None (or a minimal NodeMetadata instance if
tests need it) to satisfy the CoreNodeBase protocol; update the _FakeNode class
in the test file (referencing _FakeNode and CoreNodeBase) and import or
reference NodeMetadata as needed so the signature type is correct.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.vale/styles/Infrahub/sentence-case.yml.vale/styles/spelling-exceptions.txtAGENTS.mdchangelog/497.fixed.mddev/commands/feedback.mddev/commands/pre-ci.mddocs/AGENTS.mddocs/docs/python-sdk/guides/client.mdxdocs/docs/python-sdk/guides/python-typing.mdxdocs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdxdocs/docs/python-sdk/topics/object_file.mdxinfrahub_sdk/node/attribute.pyinfrahub_sdk/node/node.pytests/AGENTS.mdtests/unit/sdk/conftest.pytests/unit/sdk/pool/__init__.pytests/unit/sdk/pool/conftest.pytests/unit/sdk/pool/test_allocate.pytests/unit/sdk/pool/test_attribute_from_pool.pytests/unit/sdk/pool/test_pool_queries.pytests/unit/sdk/pool/test_relationship_from_pool.pytests/unit/sdk/test_attribute_generate_input_data.pytests/unit/sdk/test_client.pytests/unit/sdk/test_node.py
💤 Files with no reviewable changes (2)
- tests/unit/sdk/test_node.py
- tests/unit/sdk/test_client.py
polmichel
force-pushed
the
pmi-20260226-merge-develop-into-infrahub-develop
branch
from
97e4a8b to
52d475e
Compare
polmichel
commented
Feb 26, 2026
| ) | ||
| ip_address = clients.sync.allocate_next_ip_address( | ||
| resource_pool=ip_pool, | ||
| resource_pool=cast("CoreNodeSync", ip_pool), |
Author
There was a problem hiding this comment.
I am not sure how we should handle this case.
This is a type mismatch, but fixing this would end up changing the type of a parameter in the public method allocate_next_ip_address...
polmichel
commented
Feb 26, 2026
| ) | ||
| ip_prefix = await clients.standard.allocate_next_ip_prefix( | ||
| resource_pool=ip_pool, | ||
| resource_pool=cast("CoreNode", ip_pool), |
Author
There was a problem hiding this comment.
See upper comment but for the methodallocate_next_ip_prefix
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #856 +/- ##
====================================================
- Coverage 81.80% 80.66% -1.14%
====================================================
Files 118 118
Lines 11976 10246 -1730
Branches 2066 1534 -532
====================================================
- Hits 9797 8265 -1532
+ Misses 1551 1454 -97
+ Partials 628 527 -101
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes 🚀 New features to boost your workflow:
|
ogenstad
approved these changes
Feb 27, 2026
polmichel
deleted the
pmi-20260226-merge-develop-into-infrahub-develop
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Manual merge of
developintoinfrahub-developdue to merge conflicts. This PR replaces #852.What changed
MDXis not accepted as a spelling exceptionIt includes #850 & #829.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores