feat(protocols): support generating the SDK core protocols module [IFC-3054] - #1273
Draft
ogenstad wants to merge 1 commit into
Draft
feat(protocols): support generating the SDK core protocols module [IFC-3054]#1273ogenstad wants to merge 1 commit into
ogenstad wants to merge 1 commit into
Conversation
…C-3054] The code generator could only render protocols for a user's own schema, importing the core kinds it referenced from infrahub_sdk.protocols. It could not render that module itself: every kind in it is local, so nothing can be imported from it, both the async and sync variants share one file, and the sync classes need a suffix to keep their names distinct. Add a ProtocolTarget to select between the two, splitting the per-file header out of the per-variant body so the core module can emit one header and two bodies. Output for a user schema is unchanged, with one exception: a hierarchical kind no longer declares parent and children twice when the schema already exposes them as relationships. Golden fixtures now pin the rendered output for both variants so any further change to what infrahubctl writes has to be accepted deliberately.
Deploying infrahub-sdk-python with
|
| Latest commit: |
7e6c8d0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://50d7dbb3.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-sdk-core-protocols-ifc-3.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #1273 +/- ##
====================================================
- Coverage 84.16% 84.13% -0.03%
====================================================
Files 147 148 +1
Lines 13045 13079 +34
Branches 1930 1936 +6
====================================================
+ Hits 10979 11004 +25
- Misses 1503 1509 +6
- Partials 563 566 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrahub_sdk/protocols_generator/template.j2">
<violation number="1" location="infrahub_sdk/protocols_generator/template.j2:13">
P2: The hierarchical guard suppresses the emitted pair by checking only that `parent` is absent, then writes both `parent` and `children`. When the relationship list contains `children` but not `parent`, this re-emits a duplicate `children`; when it contains `parent` but not `children`, the `children` relationship is dropped entirely. Check each name independently so a pair member is only suppressed when it already exists as a relationship.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| {% if generic.hierarchical | default(false) %} | ||
| parent: {{ "RelationshipAttribute" | syncify(sync) }}[{{ generic.namespace + generic.name }}] | ||
| children: {{ "RelationshipManager" | syncify(sync) }}[{{ generic.namespace + generic.name }}] | ||
| {% if generic.hierarchical | default(false) and "parent" not in generic.relationships | default([]) | map(attribute="name") %} |
Contributor
There was a problem hiding this comment.
P2: The hierarchical guard suppresses the emitted pair by checking only that parent is absent, then writes both parent and children. When the relationship list contains children but not parent, this re-emits a duplicate children; when it contains parent but not children, the children relationship is dropped entirely. Check each name independently so a pair member is only suppressed when it already exists as a relationship.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/protocols_generator/template.j2, line 13:
<comment>The hierarchical guard suppresses the emitted pair by checking only that `parent` is absent, then writes both `parent` and `children`. When the relationship list contains `children` but not `parent`, this re-emits a duplicate `children`; when it contains `parent` but not `children`, the `children` relationship is dropped entirely. Check each name independently so a pair member is only suppressed when it already exists as a relationship.</comment>
<file context>
@@ -53,16 +10,16 @@ class {{ generic.namespace + generic.name }}({{core_node_name}}):
- {% if generic.hierarchical | default(false) %}
- parent: {{ "RelationshipAttribute" | syncify(sync) }}[{{ generic.namespace + generic.name }}]
- children: {{ "RelationshipManager" | syncify(sync) }}[{{ generic.namespace + generic.name }}]
+ {% if generic.hierarchical | default(false) and "parent" not in generic.relationships | default([]) | map(attribute="name") %}
+ parent: {{ "RelationshipAttribute" | syncify(sync) }}[{{ generic.namespace + generic.name }}{{ suffix }}]
+ children: {{ "RelationshipManager" | syncify(sync) }}[{{ generic.namespace + generic.name }}{{ suffix }}]
</file context>
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
1 participant
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
The code generator behind
infrahubctl protocolscan only render protocols for a user's ownschema, importing the core kinds it references from
infrahub_sdk.protocols. It cannot renderthat module itself, which is why
infrahub_sdk/protocols.pyis instead generated from a separateJinja template that lives in the Infrahub repository. That second template has drifted: it emits
bare
RelatedNode/RelationshipManagerwith no peer type, so the protocols we ship lose thepeer of every relationship, while this generator has emitted
RelationshipManager[Peer]andRelationshipAttribute[Peer]for a while now.Goal: teach this generator to render the core module too, so there is one renderer and the two
cannot drift again.
Non-goals: this PR does not regenerate
infrahub_sdk/protocols.py. Producing it needs thematching change on the Infrahub side, and it is a ~2,300-line generated diff that deserves its own
review. It follows in a second PR, with its own changelog entry.
Part of IFC-3054.
What changed
Behavioral changes, for users:
parentandchildrentwice. When the schema alreadyexposes them as relationships, which is the case for anything read from the API, they were
emitted once from the relationship list and again from the hierarchy, with the second pair
overriding the first. This is the only change to what
infrahubctl protocolswrites.Implementation notes:
ProtocolTargetselects betweenUSER_SCHEMA(the default, so every existing caller isunaffected) and
SDK_CORE.template.j2intoheader.j2, so the core module can renderone header followed by two bodies. The user branch of that header is byte-identical to before.
Synccounterpart. They are separatebecause the two positions already resolved names differently for a user schema: a peer may be
any core kind, while an inheritance list only ever switches the three names in
CORE_BASE_CLASS_TO_SYNCIFY. Generating the core module makes both sets the same, since everyclass in it is local. That asymmetry is preserved rather than fixed, to keep user output stable.
What stayed the same:
infrahubctl protocolsoutput, apart from the duplicate lines above. This is enforced, notclaimed:
tests/fixtures/protocols_generator/pins the full rendered file for both variants.The fixtures were captured from the generator before any change and the test confirmed green
on the untouched code, so the two removed lines are the entire diff.
CodeGenerator(schema=...)andrender(sync=...)are unchanged.How to review
Suggested order:
target.pyandgenerator.pyfor the mechanism.template.j2, which is only the header being removed,{{ suffix }}on the four class-namelines, and the hierarchical guard.
header.j2, where the user branch should match the block deleted fromtemplate.j2.The part worth extra scrutiny is the hierarchical guard, since it is the one behavioural change.
I chose to fix it here rather than leave it because the regenerated core module would otherwise
ship duplicated annotations for
CoreGroup,BuiltinIPPrefix,CoreMenuand others.How to test
Heads up on CI: this base is already red. On clean
origin/infrahub-developI seetest_repository_app(x2),test_task_app::test_task_list_command,test_config::test_missing_passwordfailing and atest_cliteardown error, all reproduced withthis branch's changes stashed.
Impact & rollout
duplicate
parent/childrenpair described above.the follow-up PR that regenerates
infrahub_sdk/protocols.py.Checklist
Summary by cubic
Generates the core
infrahub_sdk.protocolsmodule with the same generator used for user schemas to prevent drift and keep peer-typed relationships. Previously the generator only handled user schemas; now it also renders the core module, and it stops duplicating parent/children for hierarchical kinds when those relationships already exist.infrahubctl protocolsto drop the duplicates.ProtocolTargetwithUSER_SCHEMA(default) andSDK_CORE. Existing calls remain unchanged.SDK_CORE, emits one file containing both async and sync variants; sync classes use aSyncsuffix and the module does not import from itself.header.j2so the core module can render one header and two bodies; user header remains byte-identical.infrahub_sdk/protocols.py.Written for commit 7e6c8d0. Summary will update on new commits.