Skip to content

AAP-12516 Put node-level upstream artifacts at higher precedence than WorkflowJob.extra_vars#16379

Closed
AlanCoding wants to merge 3 commits intoansible:develfrom
AlanCoding:nested_precedence
Closed

AAP-12516 Put node-level upstream artifacts at higher precedence than WorkflowJob.extra_vars#16379
AlanCoding wants to merge 3 commits intoansible:develfrom
AlanCoding:nested_precedence

Conversation

@AlanCoding
Copy link
Copy Markdown
Member

@AlanCoding AlanCoding commented Mar 31, 2026

SUMMARY

The live test here should contain a faithful reproducer of the bug, which is kind of hard.

Resource setup:

  • outer workflow
    • job sets artifacts (saved to artifacts and ancestor_artifacts)
    • nested workflow (gets vars in extra_vars)
      • job sets new artifacts, same key (also saved to artifacts and ancestor_artifacts)
      • job prints the variable from artifacts in "always" node connection

In this real mega-function of get_job_kwargs, variables coming from the WorkflowJob are truly handled separately from the upstream (path dependent) artifacts from nodes within the same workflow.

The issue with the prior implementation, which was new as of the feature of artifact-passing-in-nested-workflows, #12223, is that a user would reasonably expect that the upstream node within the inner workflow would come at higher precedence than the upstream node in the outer workflow.

Otherwise it's usually correct that extra_vars should come at higher precedence than artifacts. However, artifacts are truly "closer" to the point of execution. I think that's a good argument, but this is still slightly opinionated when you consider that there's a single workflow case here.

  • workflow is launched with extra_vars setting "var1"
    • nodeA sets artifact of "var1" to a different value (the set_stats value)
    • nodeB, downstream of nodeA, prints it

It's important to write this out. I think I'm ok with the "set_stats" value winning. But could be arguable. With the current design & models, there is no way to distinguish these two cases.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API

Note

Medium Risk
Changes variable precedence when launching jobs from workflow nodes, which can alter runtime behavior for existing nested workflows that relied on the previous extra_vars/artifact merge order.

Overview
Fixes nested workflow variable precedence so upstream node artifacts (set_stats via ancestor_artifacts) override WorkflowJob.extra_vars when constructing downstream job extra_vars, preventing outer-workflow artifacts from incorrectly winning inside child workflows.

Adds a new live regression test (plus a small set_stats.yml playbook) that builds an outer+inner workflow chain and asserts inner workflow artifacts override outer ones while non-conflicting artifacts still propagate.

Written by Cursor Bugbot for commit 0dcc6c7. This will update automatically on new commits. Configure here.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed variable precedence in nested workflows so ancestor workflow artifacts now correctly override template-defined variables.
  • Tests

    • Added live test verifying variable propagation and precedence across nested workflows.
    • Added a test playbook used to emit and verify aggregated stats during those tests.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 31, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a33a38ca-f78a-497e-83fd-d85984e27006

📥 Commits

Reviewing files that changed from the base of the PR and between b7edc5a and 0dcc6c7.

📒 Files selected for processing (1)
  • awx/main/tests/live/tests/test_nested_workflow_artifacts.py

📝 Walkthrough

Walkthrough

Modifies the precedence ordering of extra variables in workflow job node initialization, applying workflow template special variables before ancestor artifacts so that upstream artifacts override template variables. Includes new test data and an integration test to validate the precedence behavior across nested workflows.

Changes

Cohort / File(s) Summary
Workflow Variable Precedence
awx/main/models/workflow.py
Modified WorkflowJobNode.get_job_kwargs to apply wj_special_vars before merging ancestor_artifacts (via functional_aa_dict), so ancestor artifact variables override workflow-template special vars; updated explanatory comments.
Test Data
awx/main/tests/data/projects/debug/set_stats.yml
Added Ansible test playbook using ansible.builtin.set_stats to emit stats_data for tests (local connection, conditional on stats_data).
Integration Test
awx/main/tests/live/tests/test_nested_workflow_artifacts.py
Added test_nested_workflow_set_stats_precedence live test that builds nested workflows and asserts that inner workflow artifact variables override outer/template variables while outer-only and inner-only variables propagate as expected.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: reordering variable precedence in workflow job kwargs to prioritize node-level upstream artifacts over WorkflowJob.extra_vars.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@AlanCoding AlanCoding requested a review from pb82 March 31, 2026 20:03
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@awx/main/tests/live/tests/test_nested_workflow_artifacts.py`:
- Line 4: Remove the unused import by deleting the reference to
unified_job_stdout from the import line in the test file (currently importing
from awx.main.tests.live.tests.conftest); keep wait_for_job but remove
unified_job_stdout so the import reads only the used symbol and eliminate the
unused dependency in test_nested_workflow_artifacts.py.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5705702f-fad1-4e3a-bfff-0a004aab85d1

📥 Commits

Reviewing files that changed from the base of the PR and between e80ce43 and b7edc5a.

📒 Files selected for processing (3)
  • awx/main/models/workflow.py
  • awx/main/tests/data/projects/debug/set_stats.yml
  • awx/main/tests/live/tests/test_nested_workflow_artifacts.py

@sonarqubecloud
Copy link
Copy Markdown

@AlanCoding
Copy link
Copy Markdown
Member Author

Think I prefer the other approach.

@AlanCoding AlanCoding closed this Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant