[cifmw_helpers] Fix non-deterministic parameter-dir include order - #4142
Merged
Conversation
include_dir.yml loads YAML files from a directory (e.g. artifacts/parameters, populated by many independent roles/hooks) via ansible.builtin.find, then loops over the results calling include_vars per file. find() does not guarantee sorted results (confirmed in the installed find.py: it walks with os.walk() and never sorts), so if two files in the same directory ever define the same variable, which one wins is filesystem-order dependent instead of deterministic. This is the same defect shape already fixed in ci_gen_kustomize_values' generate_values.yml (PR #4138, OSPNW-1694): a user-provided edpm_fips_mode: check override was silently discarded by a same-directory default because the combine loop's find() order wasn't sorted. That case had a live, reproduced failure; this one is preventative, not a confirmed active flake -- current writers into artifacts/parameters use distinct per-role variable prefixes (e.g. cifmw_openshift_*, cifmw_nfs_*), so no key collision was found in today's usage. It closes off the same latent class of bug for any future writer into that shared directory. Add an explicit sort(attribute='path') so include order is always deterministic (lexical path order) rather than filesystem-dependent. Verified against a real ansible-playbook run (not just static reasoning): with two files defining the same key, the fix reliably lets the lexically-last file win regardless of the underlying find() order, matching the intended override-precedence convention. Related-Issue: #OSPNW-1694 Signed-off-by: Itay Matza <imatza@redhat.com>
PR Summary by QodoMake parameter directory includes deterministic
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nemarjan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
include_dir.ymlfinds YAML files in a directory (e.g.artifacts/parameters) viaansible.builtin.find, then loops over themcalling
include_vars.find()never sorts (verified in the installedfind.py:os.walk(), nosorted()call), so if two files define thesame key, which wins is filesystem-order dependent.
Same defect shape as PR #4138
(OSPNW-1694): there,
edpm_fips_mode: checkwas silently lost to a same-directory default.That case was a live, reproduced failure. This one is preventative --
current
artifacts/parameterswriters (openshift_login,cifmw_nfs,install_yamls) each use a distinct prefix, so no active collisionexists today. This closes the same latent class before a future writer
hits it. Kept as a separate PR since it's a different role/file and
#4138 has its own unresolved CI failure to isolate.
Fix
sort(attribute='path')on the include loop, so order is deterministic.Verification
sorted()/.sort()infind.py's source.git show origin/main:...and ran itthrough real
ansible-playbookagainst colliding-key fixtures --fix reliably makes the lexically-last file win.
ansible-lintpasses.cifmw_helpersmolecule test forinclude_dir.ymlonly usesone fixture file, so it's a no-op there (no regression, but also no
CI coverage of the multi-file case this fixes).
Related-Issue: #OSPNW-1694