Refactor branch defined triggers #7696
Conversation
github-actions
bot
added
the
group/backend
WalkthroughAdds TriggerComparison (MATCH, REFRESH, UPDATE) and expands TriggerSetupReport with a refreshed list and typed filtering helpers and aggregators (triggers_with_type, modified_triggers_with_type, etc.). Reworks compare_automations to return TriggerComparison and adds gather_all_automations to page Prefect automations; callers now use gather_all_automations instead of client.read_automations. Introduces branch-aware comparison logic across computed-attribute, display-label, and HFID trigger handling; ComputedAttrJinja2TriggerDefinition gains a trigger_kind field and targets_self property to support default-branch identity checks that may skip updates. 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 |
ogenstad
force-pushed
the
pog-prefect-trigger-refactoring
branch
from
1748e45 to
1c20dd5
Compare
CodSpeed Performance ReportMerging #7696 will not alter performanceComparing Summary
|
ogenstad
force-pushed
the
pog-prefect-trigger-refactoring
branch
4 times, most recently
from
aeb4354 to
c2a923b
Compare
ogenstad
force-pushed
the
pog-prefect-trigger-refactoring
branch
from
c2a923b to
74d0115
Compare
| trigger | ||
| for trigger in report.modified_triggers_with_type(trigger_type=ComputedAttrJinja2TriggerDefinition) | ||
| if trigger.targets_self | ||
| ] |
There was a problem hiding this comment.
Previously we created a set to exclude duplicates of computed attributes that spanned multiple node kinds, with this update where we instead check for triggers that target the actual kind we can remove duplicates in a way that instead keep the correct object type of the reports.
| default_branch_triggers | ||
| and len(default_branch_triggers) == 1 | ||
| and default_branch_triggers[0].template_hash == modified_trigger.template_hash | ||
| ): |
There was a problem hiding this comment.
This pattern is the same for display_labels and HFIDs, though the fields are a bit different. Instead of always triggering tasks if the trigger has been modified (created or updated, not refreshed) we first check if we're on a non default branch and if so try to find the same trigger on the default branch and if we find it and the hash of the trigger is the same we don't schedule a recalculation to happen.
| @task(name="telemetry-gather-automations", task_run_name="Gather Automations", cache_policy=NONE) | ||
| async def gather_prefect_automations(client: PrefectClient) -> dict[str, Any]: | ||
| automations = await client.read_automations() | ||
| automations = await gather_all_automations(client=client) |
There was a problem hiding this comment.
These updates are to ensure that we can find more than the default return value for automations in the Prefect API (200)
| unchanged = self._unchanged_triggers_with_type(trigger_type=trigger_type) | ||
| return created + updated + refreshed + unchanged | ||
|
|
||
| def modified_triggers_with_type(self, trigger_type: type[T]) -> list[T]: |
There was a problem hiding this comment.
Previously we had some issues where the TriggerSetupReport contains entries of type TriggerDefinition but what we're typically interested in within the setup are specific child classes of TriggerDefinition. What we did was to either use cast or ignore the type, with this change we can get the typing correct.
| log.info( | ||
| f"Processed {trigger_log_message}: {len(report.created)} created, {len(report.updated)} updated, " | ||
| f"{len(report.refreshed)} refreshed, {len(report.unchanged)} unchanged, {len(report.deleted)} deleted" | ||
| ) |
There was a problem hiding this comment.
Aside from the changes to the input to compare_automations and its updated return values the changes in this function is mostly for readability, in order to avoid if-statements just for logging and to gather existing automations once and only filter them if required.
![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
🧹 Nitpick comments (1)
backend/infrahub/hfid/tasks.py (1)
138-145: HFID branch-aware skip logic mirrors display-label behavior; consider small naming cleanupsThe HFID setup now:
- Uses
triggers_with_type(HFIDTriggerDefinition)/modified_triggers_with_type(...)to get one direct trigger pertarget_kind, and- For non-default branches, skips
TRIGGER_UPDATE_HFIDwhen there is a single default-branch trigger for the sametarget_kindwith matchinghfid_hash.That matches the intended “only recompute when schema differs from default branch” behavior.
Minor nits for clarity:
- The comment
# Configure all DisplayLabelTriggerDefinitions in Prefectis now misleading in this HFID context.- The loop variable
display_reportwould be clearer ashfid_report(or similar).Renaming these would make the intent more obvious to future readers, but they don’t affect correctness.
Also applies to: 148-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/infrahub/computed_attribute/models.py(2 hunks)backend/infrahub/computed_attribute/tasks.py(2 hunks)backend/infrahub/display_labels/tasks.py(1 hunks)backend/infrahub/hfid/tasks.py(1 hunks)backend/infrahub/telemetry/task_manager.py(2 hunks)backend/infrahub/trigger/models.py(3 hunks)backend/infrahub/trigger/setup.py(5 hunks)backend/tests/functional/webhook/test_task.py(2 hunks)backend/tests/integration_docker/test_triggered_actions.py(2 hunks)backend/tests/unit/trigger/test_tasks.py(4 hunks)changelog/7692.fixed.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
backend/infrahub/display_labels/tasks.py (2)
backend/infrahub/trigger/models.py (2)
triggers_with_type(66-79)modified_triggers_with_type(81-92)backend/infrahub/display_labels/models.py (1)
DisplayLabelTriggerDefinition(32-163)
backend/tests/integration_docker/test_triggered_actions.py (1)
backend/infrahub/trigger/setup.py (1)
gather_all_automations(169-191)
backend/infrahub/telemetry/task_manager.py (1)
backend/infrahub/trigger/setup.py (1)
gather_all_automations(169-191)
backend/tests/unit/trigger/test_tasks.py (1)
backend/infrahub/trigger/setup.py (2)
gather_all_automations(169-191)setup_triggers(72-166)
backend/infrahub/computed_attribute/tasks.py (2)
backend/infrahub/computed_attribute/models.py (2)
ComputedAttrJinja2TriggerDefinition(118-206)targets_self(125-127)backend/infrahub/trigger/models.py (3)
triggers_with_type(66-79)modified_triggers_with_type(81-92)name(153-154)
backend/infrahub/trigger/setup.py (1)
backend/infrahub/trigger/models.py (6)
TriggerComparison(24-31)TriggerSetupReport(34-92)TriggerType(95-114)is_branch_specific(107-114)update_prefect(30-31)add_with_comparison(45-52)
backend/tests/functional/webhook/test_task.py (1)
backend/infrahub/trigger/setup.py (1)
gather_all_automations(169-191)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (21)
changelog/7692.fixed.md (1)
1-1: Changelog entry matches the implemented behaviorThe description accurately reflects the new branch-aware trigger behavior based on differing task signatures vs the default branch.
backend/infrahub/telemetry/task_manager.py (1)
11-11: Telemetry now correctly counts all Prefect automationsSwitching to
gather_all_automations(client=client)ensures telemetry sees more than Prefect’s default 200-automation limit while preserving the existing counting logic byTriggerType.Also applies to: 57-57
backend/tests/functional/webhook/test_task.py (1)
13-13: Webhook test now robust to large automation countsUsing
gather_all_automations(client=prefect_client)instead ofread_automations()keeps the assertions the same while ensuring the test still passes when Prefect has more than 200 automations.Also applies to: 213-214
backend/tests/integration_docker/test_triggered_actions.py (1)
30-31: Polling helper correctly uses gather_all_automations
wait_until_automations_are_configurednow usesgather_all_automations(client=client), which preserves the polling logic but fixes the edge case where target automations would appear after the first 200.Also applies to: 95-110
backend/tests/unit/trigger/test_tasks.py (1)
6-7: Unit tests aligned with new automation-gathering helperUsing
gather_all_automationsboth incleanup_automationand in post-setup_triggersassertions keeps tests consistent with production behavior and avoids hidden automations beyond the first page.Also applies to: 17-21, 37-39, 51-53
backend/infrahub/display_labels/tasks.py (1)
139-145: Branch-aware gating for display-label recomputation looks correct; confirm branch_name usageThe new flow:
- Uses
report.triggers_with_type(DisplayLabelTriggerDefinition)andmodified_triggers_with_type(...)to derive one “direct” trigger pertarget_kind(wheretarget_kindis set), and- For non-default branches, skips
TRIGGER_UPDATE_DISPLAY_LABELSwhen there is exactly one default-branch trigger for the sametarget_kindand itstemplate_hashmatches, which aligns with the goal of only recomputing when the schema/template actually diverges from the default branch.This is a good fit for the PR’s objective and should significantly reduce redundant recomputations on branches.
One thing to double-check: the guard
if event_name != BranchDeletedEvent.event_name and display_report.branch == branch_name:means no workflows will be submitted if
branch_nameisNone. Please confirm that all call sites ofdisplay_labels_setup_jinja2always provide a non-Nonebranch_name, or add an explicit fallback/early-return path if the “no-branch” mode is intended.Also applies to: 147-165
backend/infrahub/computed_attribute/models.py (1)
122-127: ComputedAttrJinja2TriggerDefinition metadata cleanly distinguishes self vs related triggersAdding
trigger_kindand thetargets_selfproperty (comparingtrigger_kindtocomputed_attribute.kind) provides exactly the metadata needed for downstream logic to tell “self” triggers apart from related-node triggers. Initialisation viafrom_computed_attribute(..., trigger_node=...)correctly setstrigger_kind=trigger_node.kind, so the invariant holds.If there are any other code paths that construct
ComputedAttrJinja2TriggerDefinitiondirectly, ensure they also populatetrigger_kindto keeptargets_selfreliable.Also applies to: 196-204
backend/infrahub/computed_attribute/tasks.py (2)
32-32: LGTM!The import of
ComputedAttrJinja2TriggerDefinitionis correctly added to support the new type-filtered trigger processing logic below.
316-324: LGTM!The refactored trigger gathering logic correctly uses the new type-safe helper methods and filters to
targets_selftriggers, eliminating duplicates while preserving correct object types.backend/infrahub/trigger/setup.py (6)
21-46: LGTM!The
compare_automationsfunction correctly implements the three-way comparison logic (MATCH, REFRESH, UPDATE) with appropriate handling of force_update, branch-specific triggers, and description-based refresh detection. The short-circuit evaluation in line 34 safely handles the None case fortrigger_type.
82-83: LGTM!Consolidating the log message improves consistency and maintainability.
97-104: LGTM!Using
gather_all_automationsresolves the pagination issue mentioned in the PR objectives, ensuring all automations are retrieved rather than being limited to the default 200. The prefix-based filtering correctly narrows the scope when a specific trigger type is provided.
130-136: LGTM!The comparison-based update logic correctly uses
compare_automationsto determine when Prefect updates are necessary, implementing the PR objective to reduce redundant task scheduling on branches.
161-164: LGTM!The enhanced log message now correctly reports all trigger processing outcomes, including the new "refreshed" category.
169-191: LGTM!The
gather_all_automationsfunction correctly implements pagination to retrieve all automations from Prefect, addressing the SDK's default 200-item limit mentioned in the PR objectives. The pagination logic is sound and handles the retrieval efficiently.backend/infrahub/trigger/models.py (6)
4-5: LGTM!The new imports (
StrEnum,TypeVar) and theTypeVardefinition correctly support the type-safe filtering methods introduced in this PR.Also applies to: 21-21
24-31: LGTM!The
TriggerComparisonenum cleanly models the three comparison outcomes with appropriate semantics. Theupdate_prefectproperty correctly identifies when Prefect updates are required.
36-36: LGTM!The
refreshedfield andadd_with_comparisonmethod correctly integrate the new comparison-based reporting. The updatedin_use_countproperly includes all trigger categories.Also applies to: 43-43, 45-52
54-64: LGTM!The private helper methods consistently implement type-safe filtering across all trigger categories using the
TypeVarbound toTriggerDefinition.
66-92: LGTM!The public filtering methods provide type-safe access to triggers with clear semantics:
triggers_with_typereturns all categories whilemodified_triggers_with_typefocuses on actionable changes. The docstrings clearly document the behavior.
106-114: LGTM!The
is_branch_specificproperty correctly identifies trigger types that require branch-aware comparison logic, supporting the PR's goal to optimize branch-specific trigger handling.
ogenstad
force-pushed
the
pog-prefect-trigger-refactoring
branch
from
74d0115 to
9043e0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/infrahub/trigger/models.py (1)
21-32: Typed trigger reporting/filters look solid; consider guarding futureTriggerComparisonvalues.The introduction of
TriggerComparison,TriggerSetupReport.refreshed, and the typed*_triggers_with_type/triggers_with_type/modified_triggers_with_typehelpers is a clean way to get rid of casts and to slice reports by trigger subtype;TriggerType.is_branch_specificis also a good encapsulation for the branch‑aware compare logic.One small defensive improvement:
add_with_comparisonhas explicit cases for MATCH/REFRESH/UPDATE but no fallback. If a newTriggerComparisonmember is ever added and used, it’ll silently drop on the floor. Consider adding acase _:that either raises or logs to make those mismatches obvious during development.Also applies to: 34-93, 95-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/infrahub/computed_attribute/models.py(2 hunks)backend/infrahub/computed_attribute/tasks.py(2 hunks)backend/infrahub/display_labels/tasks.py(1 hunks)backend/infrahub/hfid/tasks.py(1 hunks)backend/infrahub/telemetry/task_manager.py(2 hunks)backend/infrahub/trigger/models.py(3 hunks)backend/infrahub/trigger/setup.py(5 hunks)backend/tests/functional/webhook/test_task.py(2 hunks)backend/tests/integration_docker/test_triggered_actions.py(2 hunks)backend/tests/unit/trigger/test_tasks.py(4 hunks)changelog/7692.fixed.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/infrahub/display_labels/tasks.py
- changelog/7692.fixed.md
- backend/tests/integration_docker/test_triggered_actions.py
- backend/infrahub/hfid/tasks.py
- backend/infrahub/telemetry/task_manager.py
- backend/tests/functional/webhook/test_task.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/unit/trigger/test_tasks.py (1)
backend/infrahub/trigger/setup.py (2)
gather_all_automations(169-191)setup_triggers(72-166)
backend/infrahub/trigger/setup.py (1)
backend/infrahub/trigger/models.py (9)
TriggerComparison(24-31)TriggerSetupReport(34-92)TriggerType(95-114)is_branch_specific(107-114)name(153-154)update_prefect(30-31)generate_name(203-204)generate_name(214-215)add_with_comparison(45-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-invoke-demo-start
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-tests-functional
- GitHub Check: documentation
- GitHub Check: backend-tests-integration
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-unit
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
backend/tests/unit/trigger/test_tasks.py (1)
6-7: Tests now cover full automation gathering viagather_all_automations.Using
gather_all_automationsboth for cleanup and assertions is a good way to validate the pagination helper against Prefect’s 200‑automation limit and should prevent silent under-counting if the default API behavior changes.Also applies to: 18-21, 37-39, 51-53
backend/infrahub/computed_attribute/models.py (1)
122-127:trigger_kindfield properly wired; no other instantiations found.Verification confirms that
ComputedAttrJinja2TriggerDefinitionis instantiated only through thefrom_computed_attributeclassmethod (lines 196-210), which correctly passestrigger_kind=trigger_node.kind. No other code paths in the repository directly instantiate this class, so there is no risk of runtime failures from missing the required field.backend/infrahub/trigger/setup.py (1)
97-105: Code review is accurate; both optional robustness suggestions are valid improvements.The
gather_all_automationsfunction correctly implements pagination around Prefect's 200-item limit, and the filtering logic at lines 97-105 properly scopes automations bytrigger_type.The two optional robustness improvements are well-founded:
Infinite loop safeguard — The current
while missing_automationslogic could theoretically loop forever if/automations/filterreturns an empty page while/automations/countremains unmet. Breaking when a page yields zero items or bounding iterations tomath.ceil(automation_count / limit)is a reasonable protective measure.Type coercion — Explicitly casting
automation_count = int(automation_count_response.json())guards against API response drift (e.g., if the endpoint ever returns a string instead of an integer).The code is functional and approved; implementing these enhancements would strengthen resilience against edge cases or API behavioral shifts.
| all_triggers = report.triggers_with_type(trigger_type=ComputedAttrJinja2TriggerDefinition) | ||
|
|
||
| # Since we can have multiple trigger per NodeKind | ||
| # we need to extract the list of unique node that should be processed | ||
| unique_nodes: set[tuple[str, str, str]] = { | ||
| (trigger.branch, trigger.computed_attribute.kind, trigger.computed_attribute.attribute.name) # type: ignore[attr-defined] | ||
| for trigger in report.updated + report.created | ||
| } | ||
| for branch, kind, attribute_name in unique_nodes: | ||
| if event_name != BranchDeletedEvent.event_name and branch == branch_name: | ||
| # we need to extract the list of unique node that should be processed, this is done by filtering the triggers that targets_self | ||
| modified_triggers = [ | ||
| trigger | ||
| for trigger in report.modified_triggers_with_type(trigger_type=ComputedAttrJinja2TriggerDefinition) | ||
| if trigger.targets_self | ||
| ] | ||
|
|
||
| for modified_trigger in modified_triggers: | ||
| if event_name != BranchDeletedEvent.event_name and modified_trigger.branch == branch_name: | ||
| if branch_name != registry.default_branch: | ||
| default_branch_triggers = [ | ||
| trigger | ||
| for trigger in all_triggers | ||
| if trigger.branch == registry.default_branch | ||
| and trigger.targets_self | ||
| and trigger.computed_attribute.kind == modified_trigger.computed_attribute.kind | ||
| and trigger.computed_attribute.attribute.name | ||
| == modified_trigger.computed_attribute.attribute.name | ||
| ] | ||
| if ( | ||
| default_branch_triggers | ||
| and len(default_branch_triggers) == 1 | ||
| and default_branch_triggers[0].template_hash == modified_trigger.template_hash | ||
| ): | ||
| log.debug( | ||
| f"Skipping computed attribute updates for {modified_trigger.computed_attribute.kind}." | ||
| f"{modified_trigger.computed_attribute.attribute.name} [{branch_name}], schema is identical to default branch" | ||
| ) | ||
| continue | ||
|
|
||
| await get_workflow().submit_workflow( | ||
| workflow=TRIGGER_UPDATE_JINJA_COMPUTED_ATTRIBUTES, | ||
| context=context, | ||
| parameters={ | ||
| "branch_name": branch, | ||
| "computed_attribute_name": attribute_name, | ||
| "computed_attribute_kind": kind, | ||
| "branch_name": modified_trigger.branch, | ||
| "computed_attribute_name": modified_trigger.computed_attribute.attribute.name, | ||
| "computed_attribute_kind": modified_trigger.computed_attribute.kind, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Branch-aware Jinja2 recompute filtering looks right; verify callers and consider indexing default triggers.
The new flow that:
- narrows to
modified_triggers_with_type(...)"and thenif trigger.targets_self, and - on non‑default branches, skips scheduling when a single default‑branch self‑targeting trigger with matching kind/name and
template_hashexists,
matches the described behavior change and should prevent unnecessary recomputes on schema‑identical branches.
Two follow‑ups:
-
This loop only processes triggers where
modified_trigger.branch == branch_name. Ifcomputed_attribute_setup_jinja2can still be invoked withbranch_name=Noneor with a branch name that doesn’t match the triggers produced bysetup_triggers_specific, no recomputes will be scheduled. Please confirm all call sites now pass the concrete branch we intend to refresh. -
For installations with many triggers, recomputing
default_branch_triggersby scanningall_triggersfor everymodified_triggeris O(N²). If this ever shows up in profiling, you could pre-index default‑branch self‑targeting triggers once, e.g. keyed by(kind, attribute_name).
🏁 Script executed:
#!/bin/bash
# 1) Check how computed_attribute_setup_jinja2 is invoked.
rg -n "computed_attribute_setup_jinja2\(" -C3
# 2) If needed, identify number of COMPUTED_ATTR_JINJA2 triggers per deployment.
rg -n "ComputedAttrJinja2TriggerDefinition" -C3Length of output: 5699
🏁 Script executed:
#!/bin/bash
# Find all callers of computed_attribute_setup_jinja2
rg -n "computed_attribute_setup_jinja2" -B5 -A2 | grep -v "^--$" | head -100Length of output: 1247
🏁 Script executed:
#!/bin/bash
# Look for event handlers or workflow triggers that might invoke this function
rg -n "TRIGGER_SETUP_JINJA2_COMPUTED_ATTRIBUTES\|computed.attribute.setup.jinja2" -B3 -A3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the full function body to understand the branch_name handling
sed -n '299,400p' backend/infrahub/computed_attribute/tasks.pyLength of output: 5019
🏁 Script executed:
#!/bin/bash
# Look for how computed_attribute_setup_jinja2 is registered as a flow/task
rg -n "@flow\|@task" backend/infrahub/computed_attribute/tasks.py -A5 | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for event subscriptions or listeners that trigger this function
rg -n "BranchCreatedEvent\|BranchDeletionStarted\|SchemaBranchUpdate" -B2 -A2 -g '*.py' | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for setup_triggers calls to understand initial trigger setup
rg -n "setup_triggers" -B3 -A3 | head -100Length of output: 7665
🏁 Script executed:
#!/bin/bash
# Check models and understand TriggerBranchDefinition structure
ast-grep --pattern 'class TriggerBranchDefinition {
$$$
}' Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify how branch attribute is set on triggers
rg -n "class ComputedAttrJinja2TriggerDefinition" -A20Length of output: 1857
🏁 Script executed:
#!/bin/bash
# Check if branch_name is set at function entry point
sed -n '299,340p' backend/infrahub/computed_attribute/tasks.py | grep -E "branch_name|=|report"Length of output: 1328
🏁 Script executed:
#!/bin/bash
# Find where computed_attribute_setup_jinja2 might be invoked from event handlers
rg -n "\.submit_workflow\(.*computed.*attribute.*setup\|computed_attribute_setup_jinja2" -B5 -A5 | head -120Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for orchestration/event handling that calls this
find . -name "*.py" -type f -exec grep -l "computed_attribute_setup_jinja2\|TRIGGER_SETUP_JINJA2" {} \; 2>/dev/null | head -20Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check the event payload structure - what branch info is available
rg -n "class.*Event.*:" backend/infrahub/ -A10 | grep -i branch | head -40Length of output: 4023
🏁 Script executed:
#!/bin/bash
# Search for where workflows are submitted/triggered
rg -n "submit_workflow\|\.submit_workflow" -B5 -A5 | grep -E "JINJA|computed" -B5 -A5 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for event handlers and automation trigger setup
rg -n "branch_created_event_handler\|branch.created\|BranchCreatedEvent" -B3 -A8 | head -120Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check task_manager event handlers
rg -n "class.*Event.*Handler\|handle_event" backend/infrahub/task_manager/ -B2 -A15 | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find workflow triggering patterns
rg -n "COMPUTED_ATTRIBUTE" backend/infrahub/ -B2 -A2 | head -80Length of output: 5142
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern of always setting branch_name before calling this workflow
rg -n "branch_name\s*=" backend/infrahub/computed_attribute/ -B3 -A3 | head -100Length of output: 7869
🏁 Script executed:
#!/bin/bash
# Find where workflows are invoked in trigger definitions
rg -n "TRIGGER_COMPUTED_ATTRIBUTE\|COMPUTED_ATTRIBUTE_SETUP" backend/infrahub/computed_attribute/triggers.py -B5 -A10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check trigger catalogue to see how these workflows are called
rg -n "ComputedAttrJinja2\|COMPUTED_ATTRIBUTE_SETUP_JINJA2" backend/infrahub/trigger/catalogue.py -B5 -A10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the actual trigger definitions to see parameters passed
cat backend/infrahub/computed_attribute/triggers.py 2>/dev/null || echo "File not found"Length of output: 2303
🏁 Script executed:
#!/bin/bash
# Search for where TRIGGER_COMPUTED_ATTRIBUTE_ALL_SCHEMA is defined and used
rg -n "TRIGGER_COMPUTED_ATTRIBUTE_ALL_SCHEMA" -B3 -A8 | head -100Length of output: 2986
🏁 Script executed:
#!/bin/bash
# Find event handlers that trigger computed attribute setup
find backend/infrahub -name "*.py" -type f -exec grep -l "computed_attribute\|COMPUTED_ATTRIBUTE" {} \; | xargs grep -l "event" | head -10Length of output: 362
Add missing branch_name fallback to prevent silent trigger filtering failures.
The function is invoked via event-based triggers with branch_name extracted from event.resource['infrahub.branch.name'], but the signature allows branch_name=None (line 300). When None, the condition at line 326 (modified_trigger.branch == branch_name) never matches, causing all triggers to be silently skipped with no error.
The sibling function computed_attribute_setup_python (line 376) correctly handles this with branch_name = branch_name or registry.default_branch, but computed_attribute_setup_jinja2 omits this guard. Add the same fallback at line 305–306 to ensure consistent behavior:
branch_name = branch_name or registry.default_branchThe secondary concern about O(N²) recomputation of default_branch_triggers per modified trigger is valid but optional. If profiling later identifies it as a bottleneck, pre-indexing by (kind, attribute_name) would be straightforward.
🤖 Prompt for AI Agents
In backend/infrahub/computed_attribute/tasks.py around lines 316 to 357,
computed_attribute_setup_jinja2 can be called with branch_name=None which causes
the modified_trigger.branch == branch_name check to always fail and silently
skip triggers; add a fallback at the start of the function (around lines
305–306) to set branch_name = branch_name or registry.default_branch so None
uses the default branch, leaving the rest of the logic unchanged; optionally
consider optimizing the per-trigger lookup of default_branch_triggers later by
pre-indexing triggers by (kind, attribute_name) if profiling shows O(N²) cost.
| def compare_automations( | ||
| target: AutomationCore, existing: Automation, trigger_type: TriggerType | None, force_update: bool = False | ||
| ) -> TriggerComparison: | ||
| """Compare an AutomationCore with an existing Automation object to identify if they are identical, | ||
| if it's a branch specific automation and the branch filter may be different, or if they are different. | ||
| """ | ||
|
|
||
| if force_update: | ||
| return TriggerComparison.UPDATE | ||
|
|
||
| target_dump = target.model_dump(exclude_defaults=True, exclude_none=True) | ||
| existing_dump = existing.model_dump(exclude_defaults=True, exclude_none=True, exclude={"id"}) | ||
|
|
||
| return target_dump == existing_dump | ||
| if not trigger_type or not trigger_type.is_branch_specific: | ||
| if target_dump == existing_dump: | ||
| return TriggerComparison.MATCH | ||
| return TriggerComparison.UPDATE | ||
|
|
||
| if target_dump == existing_dump: | ||
| return TriggerComparison.MATCH | ||
|
|
||
| if target.description == existing.description: | ||
| # If only the branch related info is different, we consider it a refresh | ||
| return TriggerComparison.REFRESH | ||
|
|
||
| return TriggerComparison.UPDATE | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Branch-specific compare_automations behavior hinges on get_description; ensure all branch-scoped types encode hash there.
For non‑branch‑specific triggers, comparing the dumped AutomationCore/Automation payloads and returning MATCH/UPDATE is straightforward.
For branch‑specific types (TriggerType.is_branch_specific), the REFRESH/UPDATE split is based on:
- payloads differing, and
target.description == existing.description.
This works well for ComputedAttrJinja2TriggerDefinition, where get_description() includes template_hash, so REFRESH truly means “only branch‑filter–ish details changed, schema is identical.” For other branch‑specific trigger types (e.g., DISPLAY_LABEL_JINJA2, HUMAN_FRIENDLY_ID, Python-based ones if you ever start using TriggerSetupReport for them), this assumption only holds if their get_description() also encodes all schema‑relevant aspects (e.g., a template/hash).
It would be good to:
- verify each branch‑specific
TriggerDefinitionsubclass’sget_description()includes whatever should force a recompute when it changes, and - if any don’t, either update their descriptions (mirroring the Jinja2 pattern) or refine
compare_automationsto explicitly ignore only known branch‑filter fields instead of using description as the proxy.
Also applies to: 129-137
🏁 Script executed:
#!/bin/bash
# Inspect get_description implementations for branch-specific trigger definitions
rg -n "class .*TriggerDefinition" backend/infrahub -C3 | sed -n '1,200p'
# And search for custom get_description overrides
rg -n "def get_description" backend/infrahub -C3Length of output: 7048
🏁 Script executed:
#!/bin/bash
# Check if ComputedAttrPythonTriggerDefinition or ComputedAttrPythonQueryTriggerDefinition have get_description
rg -A 30 "class ComputedAttrPythonTriggerDefinition" backend/infrahub/computed_attribute/models.py | grep -A 15 "get_description"
rg -A 30 "class ComputedAttrPythonQueryTriggerDefinition" backend/infrahub/computed_attribute/models.py | grep -A 15 "get_description"
# Check TriggerType to see which are marked as branch-specific
rg -A 50 "class TriggerType" backend/infrahub/trigger/models.py | head -100Length of output: 1867
🏁 Script executed:
#!/bin/bash
# Check what is_branch_specific property returns
rg -n "is_branch_specific" backend/infrahub/trigger -C 5Length of output: 1672
🏁 Script executed:
#!/bin/bash
# Final verification: explicitly search for any get_description in ComputedAttrPython* classes
rg -n "class ComputedAttrPythonTriggerDefinition" backend/infrahub/computed_attribute/models.py -A 50 | head -60
rg -n "class ComputedAttrPythonQueryTriggerDefinition" backend/infrahub/computed_attribute/models.py -A 50 | head -60Length of output: 5634
ComputedAttrPythonTriggerDefinition and ComputedAttrPythonQueryTriggerDefinition lack schema encoding in get_description(); they must be updated to mirror the Jinja2/HFID pattern.
The review concern is confirmed. Of the five branch-specific trigger types:
- ✓
ComputedAttrJinja2TriggerDefinitionencodestemplate_hashinget_description() - ✓
DisplayLabelTriggerDefinitionencodestemplate_hashinget_description() - ✓
HFIDTriggerDefinitionencodeshfid_hashinget_description() - ✗
ComputedAttrPythonTriggerDefinitionuses only baseget_description()(no override) - ✗
ComputedAttrPythonQueryTriggerDefinitionuses only baseget_description()(no override)
Since compare_automations() relies on description equality to determine REFRESH vs UPDATE for branch-specific triggers, the Python types will incorrectly treat actual schema changes (e.g., changes to the Python query or transform logic) as "only branch filter changed," returning REFRESH when UPDATE is required. Both Python CA trigger definitions must override get_description() to include a hash or checksum of their schema-relevant fields (e.g., query or transform code), matching the pattern already established by the Jinja2 and HFID implementations.
| automation_count_response.raise_for_status() | ||
| automation_count: int = automation_count_response.json() | ||
| offset = 0 | ||
| limit = 200 |
There was a problem hiding this comment.
Note for later: I don't know if we have other places that would be impacted by Prefect's internal pagination limit, but should we have a wrapper on top of Prefect's client to automatically paginate such queries?
ogenstad
force-pushed
the
pog-prefect-trigger-refactoring
branch
from
9043e0b to
3d9aa65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/infrahub/computed_attribute/tasks.py (1)
316-357: Branch-aware trigger filtering looks good; add branch_name fallback to avoid silent no-opThe new flow here is solid:
- Using
report.modified_triggers_with_type(ComputedAttrJinja2TriggerDefinition)andtrigger.targets_selfnarrows down to the actual computed attributes we care about.- On non-default branches, comparing against
all_triggerson the default branch and skipping whentemplate_hashmatches avoids unnecessary recomputes while still always recomputing on the default branch itself.However, there is a latent issue with
branch_name:
- The function signature allows
branch_name: str | None = None(Line 300).- The main scheduling guard is
modified_trigger.branch == branch_name(Line 327).- If this flow is ever invoked with
branch_name=None, all triggers will be skipped silently even when there are modified default-branch triggers.- The sibling flow
computed_attribute_setup_pythonexplicitly normalizesbranch_namewithbranch_name = branch_name or registry.default_branch(Line 376), which avoids this problem there.To align the Jinja2 path with Python and prevent silent no-op executions, normalize
branch_nameat the top of the function:async def computed_attribute_setup_jinja2( context: InfrahubContext, branch_name: str | None = None, event_name: str | None = None ) -> None: database = await get_database() async with database.start_session() as db: log = get_run_logger() - if branch_name: + branch_name = branch_name or registry.default_branch + if branch_name: await add_tags(branches=[branch_name]) component = await get_component() await wait_for_schema_to_converge(branch_name=branch_name, component=component, db=db, log=log)Optionally (non-blocking), if you ever see large numbers of triggers, consider pre-indexing
all_triggersfor the default branch by(kind, attribute_name)instead of re-scanningall_triggerson every iteration of the loop.#!/bin/bash # Verify that all callers of computed_attribute_setup_jinja2 pass a non-None branch_name rg -n "computed_attribute_setup_jinja2" -C3 backend/infrahub
🧹 Nitpick comments (1)
backend/infrahub/display_labels/tasks.py (1)
140-165: Branch-aware display-label trigger filtering matches intended behaviorThis refactor lines up well with the branch-aware semantics:
all_triggers = report.triggers_with_type(DisplayLabelTriggerDefinition)plusmodified_triggers_with_typekeeps the handling strongly typed and avoids casts.- Restricting
direct_target_triggersto entries with atarget_kindensures that only direct target-node triggers drive full “update all nodes” recalculations.- On non-default branches, checking for a single default-branch trigger with the same
target_kindand matchingtemplate_hashand then skipping updates when they match correctly avoids redundant recomputes on schema-identical branches.- When the template diverges, or when no matching default-branch trigger exists, the code still submits
TRIGGER_UPDATE_DISPLAY_LABELSwithbranch_name=display_report.branchand the appropriatekind.If you later see large numbers of display-label triggers, you could optionally pre-index
all_triggersfor the default branch bytarget_kindto avoid re-scanning the full list inside the loop, but it’s not required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/infrahub/computed_attribute/models.py(2 hunks)backend/infrahub/computed_attribute/tasks.py(2 hunks)backend/infrahub/display_labels/tasks.py(1 hunks)backend/infrahub/hfid/tasks.py(1 hunks)backend/infrahub/telemetry/task_manager.py(2 hunks)backend/infrahub/trigger/models.py(3 hunks)backend/infrahub/trigger/setup.py(5 hunks)backend/tests/functional/webhook/test_task.py(2 hunks)backend/tests/integration_docker/test_triggered_actions.py(2 hunks)backend/tests/unit/trigger/test_tasks.py(4 hunks)changelog/7692.fixed.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/infrahub/hfid/tasks.py
- backend/infrahub/computed_attribute/models.py
- backend/infrahub/telemetry/task_manager.py
🧰 Additional context used
🧬 Code graph analysis (6)
backend/tests/integration_docker/test_triggered_actions.py (1)
backend/infrahub/trigger/setup.py (1)
gather_all_automations(167-189)
backend/tests/unit/trigger/test_tasks.py (1)
backend/infrahub/trigger/setup.py (2)
gather_all_automations(167-189)setup_triggers(70-164)
backend/infrahub/trigger/setup.py (1)
backend/infrahub/trigger/models.py (9)
TriggerComparison(24-31)TriggerSetupReport(34-92)TriggerType(95-114)is_branch_specific(107-114)name(153-154)update_prefect(30-31)generate_name(203-204)generate_name(214-215)add_with_comparison(45-52)
backend/tests/functional/webhook/test_task.py (1)
backend/infrahub/trigger/setup.py (1)
gather_all_automations(167-189)
backend/infrahub/display_labels/tasks.py (2)
backend/infrahub/trigger/models.py (2)
triggers_with_type(66-79)modified_triggers_with_type(81-92)backend/infrahub/display_labels/models.py (1)
DisplayLabelTriggerDefinition(32-163)
backend/infrahub/computed_attribute/tasks.py (3)
backend/infrahub/computed_attribute/models.py (2)
ComputedAttrJinja2TriggerDefinition(118-206)targets_self(125-127)backend/infrahub/trigger/models.py (3)
triggers_with_type(66-79)modified_triggers_with_type(81-92)name(153-154)backend/infrahub/git/base.py (1)
default_branch(184-185)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-invoke-demo-start
- GitHub Check: backend-tests-integration
- GitHub Check: documentation
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-tests-functional
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-unit
- GitHub Check: Cloudflare Pages
🔇 Additional comments (9)
changelog/7692.fixed.md (1)
1-1: Changelog entry matches behavioral changeThe description clearly captures the new “trigger only when task signature differs from default branch” behavior and aligns with the PR scope. No changes needed.
backend/infrahub/trigger/models.py (1)
4-6: Typed trigger helpers and TriggerComparison enum look correct and improve clarityThe additions here are coherent and helpful:
TriggerComparisoncleanly encodes MATCH/REFRESH/UPDATE, andupdate_prefectaccurately captures the “needs Prefect update” cases.TriggerSetupReport.refreshedplusadd_with_comparisonand the_..._triggers_with_typehelpers give you strongly-typed access to specific trigger subclasses without casts.triggers_with_typeandmodified_triggers_with_typecorrectly aggregate across categories and will be easy to use from call sites like computed attributes, display labels, and HFIDs.TriggerType.is_branch_specificcentralizes branch-aware trigger types, which will make future logic around branch handling more maintainable.No changes needed here.
Also applies to: 21-32, 35-44, 45-65, 66-79, 81-92, 106-115
backend/tests/integration_docker/test_triggered_actions.py (1)
30-31: Using gather_all_automations in tests aligns with production pagination behaviorSwitching from
client.read_automations()toawait gather_all_automations(client=client)inwait_until_automations_are_configuredkeeps the existing polling logic but now correctly handles cases where there are more than Prefect’s default page size of automations. This should make the test more robust on larger setups without changing its intent.Also applies to: 100-108
backend/tests/functional/webhook/test_task.py (1)
13-13: LGTM! Test updated to use pagination-aware automation retrieval.The change from
prefect_client.read_automations()togather_all_automations()ensures this test won't be affected by Prefect's default 200-item limit, aligning with the PR's objective to properly handle pagination.Also applies to: 213-213
backend/tests/unit/trigger/test_tasks.py (1)
6-6: LGTM! Consistent adoption of pagination-aware helper.All three usages in this test file correctly replace direct automation reads with
gather_all_automations(), ensuring test reliability regardless of automation count.Also applies to: 18-18, 37-37, 51-51
backend/infrahub/trigger/setup.py (4)
15-15: LGTM! Required imports for enhanced comparison logic.The added imports support the new
TriggerComparison-based workflow andTriggerSetupReportenhancements.
21-44: Well-structured refactor with clear comparison semantics.The refactored
compare_automationsfunction now returnsTriggerComparison(MATCH/REFRESH/UPDATE) instead of a boolean, providing more granular control over trigger updates. The branch-specific logic correctly distinguishes between schema changes (UPDATE) and branch-filter-only changes (REFRESH) based on description equality.Note: Past review comments already identified concerns about Python-based computed attribute triggers not encoding schema information in their descriptions, which could affect the REFRESH vs UPDATE determination for those trigger types.
80-81: LGTM! Cohesive updates supporting the new comparison workflow.The changes integrate well with the refactored comparison logic:
- Line 95 adopts
gather_all_automations()to handle pagination properly- Lines 128-134 leverage
TriggerComparisonto determine update needs via theupdate_prefectproperty- Lines 159-162 include the new "refreshed" category in logging output
Also applies to: 95-102, 128-134, 159-162
167-189: Well-implemented pagination handler solving the 200-item limit issue.The
gather_all_automationsfunction correctly addresses the core issue in the PR by:
- Fetching the total automation count first
- Paginating through results with consistent 200-item batches
- Using
>=comparison for robustness against count changes
dgarros
left a comment
dgarros
left a comment
There was a problem hiding this comment.
My comment is not a blocker since this is an important fix but it would be good to revisit soon
| retrieves them all by paginating through the results. The default within Prefect is 200 items, | ||
| and client.read_automations() doesn't support pagination parameters. | ||
| """ | ||
| automation_count_response = await client.request("POST", "/automations/count") |
There was a problem hiding this comment.
I believe we can skip this call
We should be able to identify if we have reach the end of the list by counting the number of results returned by the query
If the limit is set at 200 and we get 200 then most likely we are missing some results
There was a problem hiding this comment.
Fair enough. I'll add a note around this and update later.
This PR fixes an issue related to the tasks that are being triggered as part of schema changes related to jinja2 based computed attributes, display labels and HFIDs. The idea was that Infrahub would trigger new tasks to be executed when the schema within a branch is updated, the triggering action was when the prefect task tied to an action within a given branch was updated. A problem around this setup was that branches with the same schema as the default branch didn't contain these tasks until the schema diverged, at that point a Prefect task would be created within a branch and the act of this creation would be a trigger to re-evaluate all actions tied to that specific task. This problem combined with the fact that the Prefect SDK by default limited the automations collected to 200 would result in an invalid view of the tasks that existed in Prefect.
As part of this PR I've also implemented some other minor readability changes.
The main change is that if a task is modified in a branch (created or updated) Infrahub will only trigger a re-computation of that task if the signature of the task differs from the default branch, if it doesn't differ from the default branch data would remain as it was within the main branch.
In a follow up PR once this issue is resolved I'd like to take a look at how tests around these setups are actually managed and see what we can improve there.
Summary by CodeRabbit
Chores
Tests
Changelog
✏️ Tip: You can customize this high-level summary in your review settings.