don't allocate from pools on templates#863
don't allocate from pools on templates#863ajtmccarty wants to merge 4 commits intoinfrahub-developfrom
Conversation
WalkthroughAdds conditional logic to Node mutation-result handlers to skip processing results for template schemas. An is_template check (using TemplateSchemaAPI) is applied in both async and sync handlers; when true, updates to attributes and resource-pool–related fields are skipped. As a result, pool-allocated attributes and resource allocations on template nodes are not populated from mutation responses. No public API signatures were changed. Lines added: 20. 🚥 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. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: |
cad0ac0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bb32bd30.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://ajtm-03062026-template-pool.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #863 +/- ##
====================================================
+ Coverage 80.64% 80.72% +0.07%
====================================================
Files 118 118
Lines 10246 10256 +10
Branches 1534 1538 +4
====================================================
+ Hits 8263 8279 +16
+ Misses 1455 1452 -3
+ Partials 528 525 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
infrahub_sdk/node/node.py (1)
1093-1107: Consider skipping pool query fields for templates in_generate_mutation_query.The mutation query currently requests pool allocation fields (
attr._generate_mutation_query(),rel._generate_mutation_query()) for all nodes, including templates. Since templates never allocate from pools and_process_mutation_resultnow skips processing these fields for templates, the server response data for these fields goes unused.An optional optimization would be to add the same
isinstance(self._schema, TemplateSchemaAPI)check here to avoid requesting unnecessary data. This would reduce response payload size for template mutations.♻️ Optional: Skip pool query fields for templates
def _generate_mutation_query(self) -> dict[str, Any]: query_result: dict[str, Any] = {"ok": None, "object": {"id": None}} + is_template = isinstance(self._schema, TemplateSchemaAPI) for attr_name in self._attributes: attr: Attribute = getattr(self, attr_name) + if is_template and attr.is_from_pool_attribute(): + continue query_result["object"].update(attr._generate_mutation_query()) for rel_name in self._relationships: rel = getattr(self, rel_name) if not isinstance(rel, RelatedNode): continue + if is_template and rel.is_resource_pool: + continue query_result["object"].update(rel._generate_mutation_query()) return query_result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/node/node.py` around lines 1093 - 1107, The mutation builder _generate_mutation_query currently includes pool-allocation fields from Attribute and RelatedNode for every node; add a guard using isinstance(self._schema, TemplateSchemaAPI) to detect templates and skip calling attr._generate_mutation_query() and rel._generate_mutation_query() when true so template mutations do not request pool fields (keep building id/ok fields as before); update the loops over self._attributes and self._relationships to conditionally call the per-field _generate_mutation_query only for non-template schemas, referencing Attribute, RelatedNode, _process_mutation_result, and TemplateSchemaAPI to mirror the processing skip.
🤖 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/node.py`:
- Around line 1093-1107: The mutation builder _generate_mutation_query currently
includes pool-allocation fields from Attribute and RelatedNode for every node;
add a guard using isinstance(self._schema, TemplateSchemaAPI) to detect
templates and skip calling attr._generate_mutation_query() and
rel._generate_mutation_query() when true so template mutations do not request
pool fields (keep building id/ok fields as before); update the loops over
self._attributes and self._relationships to conditionally call the per-field
_generate_mutation_query only for non-template schemas, referencing Attribute,
RelatedNode, _process_mutation_result, and TemplateSchemaAPI to mirror the
processing skip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ba990b2-fb44-412e-9443-1b8dbb2f5fa3
📒 Files selected for processing (1)
infrahub_sdk/node/node.py
…ore value retrieved from the API
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/sdk/pool/test_template_skip_pool_allocation.py (2)
40-41: Consider a more precise assertion for template attribute preservation.The assertion
!= 42confirms the allocated value wasn't applied, but doesn't verify the expected state. Based on howAttributehandlesfrom_pooldata (defaults value toNone), asserting the actual expected value would make test failures more informative.💡 Suggested improvement
- assert node.vlan_id.value != 42 + assert node.vlan_id.value is None # Pool reference preserved, not allocatedApply the same change at line 54 for the sync variant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/sdk/pool/test_template_skip_pool_allocation.py` around lines 40 - 41, The test currently asserts node.vlan_id.value != 42 which only checks it wasn't set; change it to assert the precise expected value produced by Attribute.from_pool (i.e. assert node.vlan_id.value is None) so failures are clearer; update the same assertion in the sync variant (the second test around line 54) as well.
185-216: Fixtures don't need to be async.These fixtures don't use
awaitinternally, so they can be defined as regular sync functions. This is a minor style consideration.💡 Optional simplification
`@pytest.fixture` -async def vlan_template_schema() -> TemplateSchemaAPI: +def vlan_template_schema() -> TemplateSchemaAPI: return TemplateSchemaAPI( ... ) `@pytest.fixture` -async def device_template_schema() -> TemplateSchemaAPI: +def device_template_schema() -> TemplateSchemaAPI: return TemplateSchemaAPI( ... )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/sdk/pool/test_template_skip_pool_allocation.py` around lines 185 - 216, The fixtures vlan_template_schema and device_template_schema are declared async but do not await anything; change them to regular synchronous pytest fixtures by removing the async keyword (keep the `@pytest.fixture` decorator and return TemplateSchemaAPI instances as before) so functions vlan_template_schema and device_template_schema become normal def fixtures returning TemplateSchemaAPI constructed with AttributeSchemaAPI and RelationshipSchemaAPI values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/sdk/pool/test_template_skip_pool_allocation.py`:
- Around line 40-41: The test currently asserts node.vlan_id.value != 42 which
only checks it wasn't set; change it to assert the precise expected value
produced by Attribute.from_pool (i.e. assert node.vlan_id.value is None) so
failures are clearer; update the same assertion in the sync variant (the second
test around line 54) as well.
- Around line 185-216: The fixtures vlan_template_schema and
device_template_schema are declared async but do not await anything; change them
to regular synchronous pytest fixtures by removing the async keyword (keep the
`@pytest.fixture` decorator and return TemplateSchemaAPI instances as before) so
functions vlan_template_schema and device_template_schema become normal def
fixtures returning TemplateSchemaAPI constructed with AttributeSchemaAPI and
RelationshipSchemaAPI values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15727e0c-08cd-4264-bf3c-d36048110f18
📒 Files selected for processing (1)
tests/unit/sdk/pool/test_template_skip_pool_allocation.py
2 participants
stop trying to process pools for templates b/c we will never provision from pools on a template
Summary by CodeRabbit
Bug Fixes
Tests