refactor: handle INJECT_FACTS_AS_VARS=false by using ansible_facts instead#213
Merged
richm merged 1 commit intolinux-system-roles:mainfrom Jan 8, 2026
Merged
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. 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>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the role to stop using deprecated top-level Ansible fact variables and instead reference facts via the ansible_facts dict, updating tasks, tests, and documentation accordingly. Sequence diagram for manage_config backup using ansible_factssequenceDiagram
participant AnsibleController
participant PostfixRole
participant TargetHost
AnsibleController->>PostfixRole: run task Apply_changes
PostfixRole->>PostfixRole: check __postfix_has_config_changed
alt config_changed
PostfixRole->>PostfixRole: evaluate postfix_backup_multiple
alt postfix_backup_multiple_true
PostfixRole->>TargetHost: setup filter=ansible_date_time
TargetHost-->>PostfixRole: ansible_facts.date_time
end
PostfixRole->>PostfixRole: evaluate postfix_backup or postfix_backup_multiple
alt backup_condition_true
PostfixRole->>TargetHost: copy /etc/postfix/main.cf
Note over PostfixRole,TargetHost: dest=/etc/postfix/main.cf.ansible_facts[date_time][iso8601] or backup
TargetHost-->>PostfixRole: backup_result
end
end
Flow diagram for distro specific vars selection using ansible_factsflowchart TD
A["Start role execution"] --> B["Read ansible_facts"]
B --> C["Compute os_family = ansible_facts['os_family']"]
B --> D["Compute distribution = ansible_facts['distribution']"]
B --> E["Compute distribution_major_version = ansible_facts['distribution_major_version']"]
B --> F["Compute distribution_version = ansible_facts['distribution_version']"]
C --> G["Try vars file os_family.yml"]
D --> H["Try vars file distribution.yml"]
D --> I["Try vars file distribution_distribution_major_version.yml"]
D --> J["Try vars file distribution_distribution_version.yml"]
G --> K["Include file if __postfix_vars_file is file"]
H --> K
I --> K
J --> K
K --> L["Set distro flags using ansible_facts['distribution'] in __postfix_rh_distros"]
L --> M["Set distro flags using ansible_facts['distribution'] in __postfix_rh_distros_fedora"]
M --> N["End"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
[citest] |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In vars and tests where you now use
ansible_facts['distribution']in conditionals, consider usingansible_facts.get('distribution')(optionally with a default) to avoid hard failures if facts are not gathered for some reason. - The updated
include_varsloop intasks/set_facts.ymlrepeatsansible_facts['distribution']several times; you could assign this to a temporary var viaset_factorvarsto improve readability and reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In vars and tests where you now use `ansible_facts['distribution']` in conditionals, consider using `ansible_facts.get('distribution')` (optionally with a default) to avoid hard failures if facts are not gathered for some reason.
- The updated `include_vars` loop in `tasks/set_facts.yml` repeats `ansible_facts['distribution']` several times; you could assign this to a temporary var via `set_fact` or `vars` to improve readability and reduce duplication.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
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.
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 Ansible role and documentation to use the ansible_facts dictionary instead of legacy fact variables for compatibility with INJECT_FACTS_AS_VARS=false.
Bug Fixes:
Enhancements:
Documentation:
Tests: