test: ensure role gathers the facts it uses by having test clear_facts before include_role#845
Conversation
Reviewer's GuideTest harness is updated so every network role invocation is run via a new helper task file that clears Ansible facts first, and all test playbooks are refactored to use precomputed platform helper facts and the helper task instead of directly importing the role or referencing ansible_facts in when-conditions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
merge_ansible_factsmodule updatessaved_ansible_factsin-place (saved.update(current)); if callers ever re-use the originalsaved_factsvalue elsewhere this could introduce subtle side effects, so consider copying (saved = saved.copy()) before updating. - The
MINIMUM_NM_VERSION_CHECKtemplate string was changed to put the condition on the same line as the opening triple quote, which makes its indentation brittle and harder to read compared to the other multiline YAML snippets; it may be clearer and safer to keep a leading newline and consistent indentation like the other constants. - The repeated
include_vars: vars/rh_distros_vars.ymlplusset_factblock to derive__network_*platform facts appears in many test playbooks; consider moving this into a small shared task file and including it to reduce duplication and the risk of divergence between files.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `merge_ansible_facts` module updates `saved_ansible_facts` in-place (`saved.update(current)`); if callers ever re-use the original `saved_facts` value elsewhere this could introduce subtle side effects, so consider copying (`saved = saved.copy()`) before updating.
- The `MINIMUM_NM_VERSION_CHECK` template string was changed to put the condition on the same line as the opening triple quote, which makes its indentation brittle and harder to read compared to the other multiline YAML snippets; it may be clearer and safer to keep a leading newline and consistent indentation like the other constants.
- The repeated `include_vars: vars/rh_distros_vars.yml` plus `set_fact` block to derive `__network_*` platform facts appears in many test playbooks; consider moving this into a small shared task file and including it to reduce duplication and the risk of divergence between files.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #845 +/- ##
==========================================
+ Coverage 43.11% 43.18% +0.06%
==========================================
Files 12 12
Lines 3124 3126 +2
==========================================
+ Hits 1347 1350 +3
+ Misses 1777 1776 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7ad7f7d to
82431bc
Compare
|
[citest] |
|
[citest] |
c5a0984 to
1e4391c
Compare
|
[citest] |
|
[citest] |
|
[citest] |
|
[citest] |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- run_role_with_clear_facts.yml is documented as saving/restoring facts but only calls
meta: clear_factsand then runs the role; either implement the save/restore behavior or update the header comment so it accurately describes what the task file does. - The repeated "Include distro variables" / "Set platform facts" blocks across many test playbooks are identical; consider extracting them into a shared task file (e.g. tests/tasks/set_platform_facts.yml) to reduce duplication and keep future changes to the platform predicates in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- run_role_with_clear_facts.yml is documented as saving/restoring facts but only calls `meta: clear_facts` and then runs the role; either implement the save/restore behavior or update the header comment so it accurately describes what the task file does.
- The repeated "Include distro variables" / "Set platform facts" blocks across many test playbooks are identical; consider extracting them into a shared task file (e.g. tests/tasks/set_platform_facts.yml) to reduce duplication and keep future changes to the platform predicates in one place.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…s before include_role The role gathers the facts it uses. For example, if the user uses `ANSIBLE_GATHERING=explicit`, the role uses the `setup` module with the facts and subsets it requires. This change allows us to test this. Before every role invocation, the test will use `meta: clear_facts` so that the role starts with no facts. Create a task file tests/tasks/run_role_with_clear_facts.yml to do the tasks to clear the facts and run the role. Note that this means we don't need to use `gather_facts` for the tests. Some vars defined using `ansible_facts` have been changed to be defined with `set_fact` instead. This is because of the fact that `vars` are lazily evaluated - the var might be referenced when the facts have been cleared, and will issue an error like `ansible_facts["distribution"] is undefined`. This is typically done for blocks that have a `when` condition that uses `ansible_facts` and the block has a role invocation using run_role_with_clear_facts.yml These have been rewritten to define the `when` condition using `set_fact`. This is because the `when` condition is evaluated every time a task is invoked in the block, and if the facts are cleared, this will raise an undefined variable error. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
|
[citest] |
The role gathers the facts it uses. For example, if the user uses
ANSIBLE_GATHERING=explicit, the role uses thesetupmodule with thefacts and subsets it requires.
This change allows us to test this. Before every role invocation, the test
will use
meta: clear_factsso that the role starts with no facts.Create a task file tests/tasks/run_role_with_clear_facts.yml to do the tasks
to clear the facts and run the role. Note that this means we don't need to
use
gather_factsfor the tests.Some vars defined using
ansible_factshave been changed to be defined withset_factinstead. This is because of the fact thatvarsare lazilyevaluated - the var might be referenced when the facts have been cleared, and
will issue an error like
ansible_facts["distribution"] is undefined. This istypically done for blocks that have a
whencondition that usesansible_factsand the block has a role invocation using run_role_with_clear_facts.yml
These have been rewritten to define the
whencondition usingset_fact. Thisis because the
whencondition is evaluated every time a task is invoked in theblock, and if the facts are cleared, this will raise an undefined variable error.
Summary by Sourcery
Introduce a reusable test task to run the linux-system-roles.network role after clearing facts, and update network tests to use fact-independent conditions and this new helper.
Tests: