test: ensure role gathers the facts it uses by having test clear_facts before include_role#593
Conversation
Reviewer's GuideTest playbooks are refactored to run the storage role via a new helper task file that clears ansible facts before each role invocation, while the role’s main task and supporting vars are updated to avoid depending on pre-gathered facts or explicit service_facts, and some fact-dependent logic is moved to set_fact for safe lazy evaluation. Sequence diagram for tests running storage role with cleared factssequenceDiagram
actor Tester
participant TestPlaybook as tests_*.yml
participant Helper as run_role_with_clear_facts.yml
participant Ansible as Ansible_engine
participant Role as storage_role
participant Setup as setup_module
participant ServiceFacts as service_facts_module
Tester->>TestPlaybook: ansible-playbook tests_*.yml
TestPlaybook->>Helper: include_tasks run_role_with_clear_facts.yml
Helper->>Ansible: meta clear_facts
Ansible-->>Helper: ansible_facts cleared
Helper->>Role: include_role storage
Role->>Setup: gather required fact subsets
Setup-->>Role: minimal ansible_facts
Role->>Role: compute storage_cryptsetup_services using ansible_facts.services if present
alt services not in ansible_facts and cryptsetup_services not skipped
Role->>ServiceFacts: gather service facts
ServiceFacts-->>Role: ansible_facts.services populated
Role->>Role: recompute storage_cryptsetup_services
end
Role->>Role: manage storage devices and mask cryptsetup services
Role-->>Helper: role tasks completed
Helper-->>TestPlaybook: role run finished
TestPlaybook-->>Tester: test assertions and results
Flow diagram for storage role cryptsetup service management with conditional fact gatheringflowchart TD
A[start role task main_blivet] --> B[Define storage_cryptsetup_services from ansible_facts.services if present else empty list]
B --> C{services in ansible_facts?}
C -->|yes| D[storage_cryptsetup_services based on existing ansible_facts.services]
C -->|no| E[storage_cryptsetup_services set to empty list]
D --> F{cryptsetup_services in storage_skip_checks?}
E --> F
F -->|yes| G[Skip service_facts and masking tasks]
F -->|no| H[Run service_facts task]
H --> I[ansible_facts.services populated]
I --> J[Reevaluate storage_cryptsetup_services inside block using updated ansible_facts.services]
J --> K[Mask systemd cryptsetup services]
K --> L[Unmask systemd cryptsetup services]
G --> M[end role task main_blivet]
L --> M
File-Level Changes
Possibly linked issues
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:
- In
merge_ansible_facts, you updatesavedin place; if callers reusesaved_factselsewhere this can be surprising—consider copying (e.g.saved = copy.deepcopy(saved)beforeupdate) and/or documenting that the input dict will be mutated. - The new
service_factscondition intasks/main-blivet.yml(when: not "services" in ansible_facts) changes semantics from the previousstorage_skip_checks-based gate; if the intent is still to allow explicit skipping, you may want to preserve the skip list check and layer theservicespresence check on top of it. - In
tests/tasks/run_role_with_clear_facts.yml, if the included role fails aftermeta: clear_facts, the final merge step will be skipped and the play will continue with cleared facts; wrapping the role include and merge in ablockwithalwaysfor the merge step would make fact restoration more robust on failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `merge_ansible_facts`, you update `saved` in place; if callers reuse `saved_facts` elsewhere this can be surprising—consider copying (e.g. `saved = copy.deepcopy(saved)` before `update`) and/or documenting that the input dict will be mutated.
- The new `service_facts` condition in `tasks/main-blivet.yml` (`when: not "services" in ansible_facts`) changes semantics from the previous `storage_skip_checks`-based gate; if the intent is still to allow explicit skipping, you may want to preserve the skip list check and layer the `services` presence check on top of it.
- In `tests/tasks/run_role_with_clear_facts.yml`, if the included role fails after `meta: clear_facts`, the final merge step will be skipped and the play will continue with cleared facts; wrapping the role include and merge in a `block` with `always` for the merge step would make fact restoration more robust on failures.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 #593 +/- ##
==========================================
- Coverage 16.54% 10.33% -6.22%
==========================================
Files 2 8 +6
Lines 284 2023 +1739
Branches 79 0 -79
==========================================
+ Hits 47 209 +162
- Misses 237 1814 +1577
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
16e7bf3 to
457b0ca
Compare
|
[citest] |
|
@spetrosi new changes
|
|
[citest] |
|
[citest_bad] |
24688cb to
2d5c007
Compare
|
[citest] |
|
[citest] |
|
[citest] |
f22b519 to
dc359b7
Compare
…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] |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
tests/tasks/run_role_with_clear_facts.ymlheader and comments claim to save and restore facts and mergeansible_facts, but the implementation only clears facts and runs the role; either implement the described save/restore behavior or update the comments to match what the task file actually does. - In
tasks/main-blivet.ymlthe skip mechanism has effectively changed from usingservice_factstocryptsetup_services; if this rename is intentional, consider aligning the string used instorage_skip_checks(and existing callers) to avoid silently changing skip behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `tests/tasks/run_role_with_clear_facts.yml` header and comments claim to save and restore facts and merge `ansible_facts`, but the implementation only clears facts and runs the role; either implement the described save/restore behavior or update the comments to match what the task file actually does.
- In `tasks/main-blivet.yml` the skip mechanism has effectively changed from using `service_facts` to `cryptsetup_services`; if this rename is intentional, consider aligning the string used in `storage_skip_checks` (and existing callers) to avoid silently changing skip behavior.
## Individual Comments
### Comment 1
<location path="tasks/main-blivet.yml" line_range="62-64" />
<code_context>
+ vars:
+ # rejectattr required because the fix to service_facts is on Ansible > 2.12 only
+ # https://github.com/ansible/ansible/pull/75326
storage_cryptsetup_services: "{{
ansible_facts.services.values() |
selectattr('name', 'defined') |
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a block-level `vars` for `storage_cryptsetup_services` prevents it from seeing services gathered by `service_facts` in the same block
Because block-level `vars` are evaluated before any tasks in the block run, `storage_cryptsetup_services` is computed once, when `ansible_facts` still lacks `services`. It becomes `[]` via the `if 'services' in ansible_facts else []` guard and is never recomputed after `service_facts` populates `ansible_facts.services`, so the later `systemd` masking step sees an empty list and is a no-op on the first run.
To have `storage_cryptsetup_services` use the newly gathered service facts, either move this expression into a `set_fact` that runs after `service_facts`, or define it as task-level `vars` on the `systemd` (or other consuming) tasks so it evaluates after `service_facts` has executed. Otherwise, cryptsetup services will be skipped when service facts weren’t pre-existing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| storage_cryptsetup_services: "{{ | ||
| ansible_facts.services.values() | | ||
| selectattr('name', 'defined') | |
There was a problem hiding this comment.
issue (bug_risk): Using a block-level vars for storage_cryptsetup_services prevents it from seeing services gathered by service_facts in the same block
Because block-level vars are evaluated before any tasks in the block run, storage_cryptsetup_services is computed once, when ansible_facts still lacks services. It becomes [] via the if 'services' in ansible_facts else [] guard and is never recomputed after service_facts populates ansible_facts.services, so the later systemd masking step sees an empty list and is a no-op on the first run.
To have storage_cryptsetup_services use the newly gathered service facts, either move this expression into a set_fact that runs after service_facts, or define it as task-level vars on the systemd (or other consuming) tasks so it evaluates after service_facts has executed. Otherwise, cryptsetup services will be skipped when service facts weren’t pre-existing.
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
Ensure storage role tests run with cleared Ansible facts so the role gathers required facts itself before execution.
Enhancements:
Tests: