AAP-12516 [option 2] Handle nested workflow artifacts via root node ancestor_artifacts#16381
Open
AlanCoding wants to merge 7 commits intoansible:develfrom
Open
AAP-12516 [option 2] Handle nested workflow artifacts via root node ancestor_artifacts#16381AlanCoding wants to merge 7 commits intoansible:develfrom
ancestor_artifacts#16381AlanCoding wants to merge 7 commits intoansible:develfrom
Conversation
|
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAncestor-artifact propagation for nested workflows was changed: root nodes of nested workflow jobs are now seeded with Changes
Sequence DiagramsequenceDiagram
participant ParentWF as Parent Workflow Job
participant Scheduler as Task Manager
participant SpawnedJob as Spawned Unified Job
participant NestedRoot as Root Node of Nested Workflow
ParentWF->>Scheduler: request spawn_node (includes ancestor_artifacts)
Scheduler->>SpawnedJob: create_unified_job(...)
Scheduler->>SpawnedJob: save job
alt spawn_node.ancestor_artifacts && unified_job_template is WorkflowJobTemplate
Scheduler->>SpawnedJob: seed_root_ancestor_artifacts(artifacts)
SpawnedJob->>NestedRoot: set ancestor_artifacts on root nodes
end
Scheduler->>Scheduler: continue launch checks and signaling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
awx/main/scheduler/task_manager.py (1)
244-245: Prefer explicit nested-workflow seeding over truthiness-gated seeding.Using truthiness on
ancestor_artifactsmeans{}skips this path. Making the nested-workflow call unconditional (with a safe default) keeps behavior explicit and less fragile.Proposed refactor
- if spawn_node.ancestor_artifacts and isinstance(spawn_node.unified_job_template, WorkflowJobTemplate): - job.seed_root_ancestor_artifacts(spawn_node.ancestor_artifacts) + if isinstance(spawn_node.unified_job_template, WorkflowJobTemplate): + job.seed_root_ancestor_artifacts(spawn_node.ancestor_artifacts or {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/scheduler/task_manager.py` around lines 244 - 245, Replace the truthiness check on spawn_node.ancestor_artifacts with an explicit call that always seeds a safe default: keep the WorkflowJobTemplate type check (isinstance(spawn_node.unified_job_template, WorkflowJobTemplate)) and unconditionally call job.seed_root_ancestor_artifacts(spawn_node.ancestor_artifacts or {}) so empty dicts no longer skip seeding but None still results in an empty dict being passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@awx/main/scheduler/task_manager.py`:
- Around line 244-245: Replace the truthiness check on
spawn_node.ancestor_artifacts with an explicit call that always seeds a safe
default: keep the WorkflowJobTemplate type check
(isinstance(spawn_node.unified_job_template, WorkflowJobTemplate)) and
unconditionally call
job.seed_root_ancestor_artifacts(spawn_node.ancestor_artifacts or {}) so empty
dicts no longer skip seeding but None still results in an empty dict being
passed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58d9b2c7-b0b1-4bf2-9333-1360538c8448
📒 Files selected for processing (4)
awx/main/models/workflow.pyawx/main/scheduler/task_manager.pyawx/main/tests/data/projects/debug/set_stats.ymlawx/main/tests/live/tests/test_nested_workflow_artifacts.py
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
awx/main/tests/live/tests/test_nested_workflow_artifacts.py (1)
100-106: Assert terminal success states afterwait_for_jobfor clearer failures.
wait_for_jobguarantees completion, not correctness. Adding explicit success assertions for spawned workflow/job nodes will make failures deterministic and easier to triage in CI.Suggested patch
@@ outer_wfj = outer_wft.create_unified_job() outer_wfj.signal_start() wait_for_job(outer_wfj, running_timeout=120) + outer_wfj.refresh_from_db() + assert outer_wfj.status == 'successful', f'Outer workflow failed with status={outer_wfj.status}' @@ inner_wfj = inner_wf_node.job assert inner_wfj is not None, 'Inner workflow job was never created' + inner_wfj.refresh_from_db() + assert inner_wfj.status == 'successful', f'Inner workflow failed with status={inner_wfj.status}' @@ reader_node = inner_wfj.workflow_job_nodes.get(identifier='reader') assert reader_node.job is not None, 'Reader job was never created' + reader_node.job.refresh_from_db() + assert reader_node.job.status == 'successful', f'Reader job failed with status={reader_node.job.status}' @@ wfj = wft.create_unified_job() wfj.signal_start() wait_for_job(wfj, running_timeout=120) + wfj.refresh_from_db() + assert wfj.status == 'successful', f'Workflow failed with status={wfj.status}'As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 196-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/live/tests/test_nested_workflow_artifacts.py` around lines 100 - 106, The test calls wait_for_job(outer_wfj, ...) which only ensures completion but not success; after waiting assert that outer_wfj.status (or outer_wfj.node_instance.status if applicable) equals the successful terminal state and likewise assert inner_wfj.status == 'successful' (or use the project's canonical success sentinel) after obtaining inner_wf_node and inner_wfj to make failures deterministic; apply the same pattern for the similar block around lines 196-200 by adding explicit success assertions for the spawned workflow/job nodes (use the exact attributes used elsewhere in tests, e.g., outer_wfj.status and inner_wfj.status or their .status_text/.summary fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@awx/main/tests/live/tests/test_nested_workflow_artifacts.py`:
- Around line 100-106: The test calls wait_for_job(outer_wfj, ...) which only
ensures completion but not success; after waiting assert that outer_wfj.status
(or outer_wfj.node_instance.status if applicable) equals the successful terminal
state and likewise assert inner_wfj.status == 'successful' (or use the project's
canonical success sentinel) after obtaining inner_wf_node and inner_wfj to make
failures deterministic; apply the same pattern for the similar block around
lines 196-200 by adding explicit success assertions for the spawned workflow/job
nodes (use the exact attributes used elsewhere in tests, e.g., outer_wfj.status
and inner_wfj.status or their .status_text/.summary fields).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5685869f-88fa-4924-b29e-f87749f7234e
📒 Files selected for processing (2)
awx/main/models/workflow.pyawx/main/tests/live/tests/test_nested_workflow_artifacts.py
✅ Files skipped from review due to trivial changes (1)
- awx/main/models/workflow.py
pb82
approved these changes
Apr 1, 2026
|
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.
SUMMARY
I didn't love
#16379
because it changed an existing story in an unintended way.
This adds an additional test, and preserves the behavior in that prior case.
This is also more philosophically satisfying. As you are processing a root node, you can not easily grab the node that generated that workflow in the parent workflow. So when that parent workflow's node generates the nested workflow, this has it apply
ancestor_artifactsto the root nodes of the child. This is much more intuitively consistent. It shifts the logic location a little bit, but overall much more satisfying.ISSUE TYPE
COMPONENT NAME
Note
Medium Risk
Changes how
set_statsartifacts are propagated into nested workflows by moving propagation to runtime seeding of child workflow root nodes, which can affect variable precedence and workflow/job behavior. Updates touch workflow scheduling/job spawning paths, so regressions could impact workflow launches and extra_vars merging.Overview
Fixes nested workflow artifact precedence by stopping parent workflow artifacts from being merged into a child workflow’s
extra_varsand instead seeding them onto the child workflow’s root nodes’ancestor_artifactsso they propagate normally.Adds
WorkflowJob.seed_root_ancestor_artifacts()and calls it from the task manager when spawning a nested workflow, and updatesWorkflowJobNode.get_job_kwargs()to initialize from pre-seeded artifacts (excludingjob_slice) and only apply artifacts intoextra_varsforJobTemplatenodes.Adds a small
set_stats.ymlplaybook plus live tests covering (1) nested workflowset_statsoverride behavior and (2) workflow-levelextra_varscontinuing to override artifacts.Written by Cursor Bugbot for commit 1c5d0f2. This will update automatically on new commits. Configure here.
Summary by CodeRabbit