test: add tasks and action plugin to run role with clear facts [citest_skip]#321
Merged
richm merged 1 commit intolinux-system-roles:mainfrom Mar 19, 2026
Merged
Conversation
Reviewer's GuideIntroduce a reusable test task wrapper that clears and later restores Ansible facts around role execution, plus an action plugin to merge facts, and update certificate role tests to use this wrapper instead of including the role directly so the role’s own fact-gathering logic is exercised. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In tests/tasks/run_role_with_clear_facts.yml,
saved_facts: "{{ ansible_facts }}"will fail if the play is ever run withgather_facts: false; consider using a default (e.g.{{ ansible_facts | default({}) }}) and mirroring that assumption in themerge_ansible_factsplugin input validation. - The helper task file and plugin introduce a generic
saved_factsvariable name; to avoid accidental collisions with existing test variables, consider renaming it with a more specific prefix (e.g.__sr_saved_facts). - The comment in run_role_with_clear_facts.yml refers to
tests/librarybeing on the module path, but the implementation uses an action plugin undertests/action_plugins; it would be clearer to update the comment to match the actual plugin location/mechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In tests/tasks/run_role_with_clear_facts.yml, `saved_facts: "{{ ansible_facts }}"` will fail if the play is ever run with `gather_facts: false`; consider using a default (e.g. `{{ ansible_facts | default({}) }}`) and mirroring that assumption in the `merge_ansible_facts` plugin input validation.
- The helper task file and plugin introduce a generic `saved_facts` variable name; to avoid accidental collisions with existing test variables, consider renaming it with a more specific prefix (e.g. `__sr_saved_facts`).
- The comment in run_role_with_clear_facts.yml refers to `tests/library` being on the module path, but the implementation uses an action plugin under `tests/action_plugins`; it would be clearer to update the comment to match the actual plugin location/mechanism.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>
richm
changed the title
Collaborator
Author
|
[citest] |
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
1 participant
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.
The file tests/tasks/run_role_with_clear_facts.yml allows us to invoke the
role with no ansible_facts in order to test that the role is able to gather
its own facts if the user is using
ANSIBLE_GATHERING=explicit. However,the test may need facts, so this task will save the current facts, then clear
them, then run the role, then merge the saved facts with any facts set by
the role. The action plugin is needed to merge
ansible_factsbecause thatis a "special" variable and cannot be modified with
set_fact,update_fact,etc.