-
Notifications
You must be signed in to change notification settings - Fork 13
refactor: handle INJECT_FACTS_AS_VARS=false by using ansible_facts instead #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…stead Ansible 2.20 has deprecated the use of Ansible facts as variables. For example, `ansible_distribution` is now deprecated in favor of `ansible_facts["distribution"]`. This is due to making the default setting `INJECT_FACTS_AS_VARS=false`. For now, this will create WARNING messages, but in Ansible 2.24 it will be an error. Set the bootloader_facts variable separately since it will not be set automatically. See https://docs.ansible.com/projects/ansible/latest/porting_guides/porting_guide_core_2.20.html#inject-facts-as-vars Signed-off-by: Rich Megginson <rmeggins@redhat.com>
|
[citest] |
Reviewer's GuideRefactors the role and its tests to stop relying on auto-injected Ansible fact variables and instead use the ansible_facts dictionary explicitly, and adds an explicit set_fact workaround so bootloader_facts remains available when INJECT_FACTS_AS_VARS is false. Sequence diagram for bootloader_facts collection and workaroundsequenceDiagram
actor User
participant AnsibleController
participant RoleTasksMain as Role_tasks_main_yml
participant BootloaderFactsModule as bootloader_facts_module
User->>AnsibleController: Run_play_with_role
AnsibleController->>RoleTasksMain: Execute_tasks
alt bootloader_gather_facts_is_true
RoleTasksMain->>BootloaderFactsModule: bootloader_facts
BootloaderFactsModule-->>RoleTasksMain: ansible_facts.bootloader_facts
RoleTasksMain->>RoleTasksMain: set_fact bootloader_facts
Note over RoleTasksMain: bootloader_facts := ansible_facts['bootloader_facts']
else bootloader_gather_facts_is_false
RoleTasksMain->>RoleTasksMain: Skip_fact_collection_and_set_fact
end
RoleTasksMain-->>AnsibleController: Role_completed
AnsibleController-->>User: Report_status
Flow diagram for using ansible_facts and setting bootloader_factsflowchart TD
Start["Start role execution"] --> CheckArch["Check architecture: ansible_facts['architecture'] == 's390x'?"]
CheckArch -->|Yes| SkipRole["Fail role for unsupported s390x"]
CheckArch -->|No| ContinueRole["Continue role tasks"]
ContinueRole --> CheckGather["bootloader_gather_facts | bool?"]
CheckGather -->|False| EndNoFacts["Skip bootloader facts collection"]
CheckGather -->|True| CollectFacts["Run bootloader_facts module"]
CollectFacts --> StoreFacts["ansible_facts['bootloader_facts'] available"]
StoreFacts --> SetFactWorkaround["set_fact bootloader_facts := ansible_facts['bootloader_facts']"]
SetFactWorkaround --> EndWithFacts["bootloader_facts variable available even when INJECT_FACTS_AS_VARS=false"]
EndNoFacts --> End["End role"]
EndWithFacts --> End
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
==========================================
+ Coverage 78.43% 86.90% +8.47%
==========================================
Files 2 2
Lines 255 275 +20
==========================================
+ Hits 200 239 +39
+ Misses 55 36 -19
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- For the new
Set bootloader_facts variabletask, consider guarding against missingansible_facts['bootloader_facts'](e.g.bootloader_facts: "{{ ansible_facts['bootloader_facts'] | default({}) }}") in case thebootloader_factsmodule is unavailable or fails to populate facts. - In places where you now use
ansible_facts['architecture']inwhenconditions, double-check whether facts are always guaranteed to be gathered for these tasks; if not, mirroring the defensive check used inskip_on_s390x.yml(verifying the key exists) would avoid undefined-variable issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the new `Set bootloader_facts variable` task, consider guarding against missing `ansible_facts['bootloader_facts']` (e.g. `bootloader_facts: "{{ ansible_facts['bootloader_facts'] | default({}) }}"`) in case the `bootloader_facts` module is unavailable or fails to populate facts.
- In places where you now use `ansible_facts['architecture']` in `when` conditions, double-check whether facts are always guaranteed to be gathered for these tasks; if not, mirroring the defensive check used in `skip_on_s390x.yml` (verifying the key exists) would avoid undefined-variable issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Ansible 2.20 has deprecated the use of Ansible facts as variables. For
example,
ansible_distributionis now deprecated in favor ofansible_facts["distribution"]. This is due to making the defaultsetting
INJECT_FACTS_AS_VARS=false. For now, this will create WARNINGmessages, but in Ansible 2.24 it will be an error.
Set the bootloader_facts variable separately since it will not be set
automatically.
See https://docs.ansible.com/projects/ansible/latest/porting_guides/porting_guide_core_2.20.html#inject-facts-as-vars
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Update role and tests to use ansible_facts instead of injected fact variables and ensure bootloader_facts is explicitly set when gathering facts.
Bug Fixes:
Enhancements:
Tests: