test: ensure role gathers the facts it uses by having test clear_facts before include_role#5
Closed
richm wants to merge 1 commit intolinux-system-roles:mainfrom
Closed
test: ensure role gathers the facts it uses by having test clear_facts before include_role#5richm wants to merge 1 commit intolinux-system-roles:mainfrom
richm wants to merge 1 commit intolinux-system-roles:mainfrom
Conversation
Reviewer's GuideTests now invoke linux-system-roles.trustee_attestation_server through a reusable helper task that clears facts before including the role, ensuring the role gathers required facts even when ANSIBLE_GATHERING is explicit, and related fact-dependent vars are refactored to rely on set_fact. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
[citest] |
![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:
- The new
tests/tasks/run_role_with_clear_facts.ymland the updated tests now hard-code the role namelinux-system-roles.trustee_attestation_server, whereastests_default.ymlpreviously referencedlinux-system-roles.template; if this helper is meant to be generic or re-used across roles, consider parameterizing the role name instead of hard-coding it. - The header comment in
run_role_with_clear_facts.ymlstill refers to "run linux-system-roles.trustee_attestation_server"; if the intent is to make this a generic helper for running roles with cleared facts, updating the comment and interface to reflect that would make its purpose clearer and reduce confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `tests/tasks/run_role_with_clear_facts.yml` and the updated tests now hard-code the role name `linux-system-roles.trustee_attestation_server`, whereas `tests_default.yml` previously referenced `linux-system-roles.template`; if this helper is meant to be generic or re-used across roles, consider parameterizing the role name instead of hard-coding it.
- The header comment in `run_role_with_clear_facts.yml` still refers to "run linux-system-roles.trustee_attestation_server"; if the intent is to make this a generic helper for running roles with cleared facts, updating the comment and interface to reflect that would make its purpose clearer and reduce confusion.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>
Contributor
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 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.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Tests: