Skip to content

refactor: handle INJECT_FACTS_AS_VARS=false by using ansible_facts instead #314

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.

Sign up for GitHub

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

Merged
richm merged 1 commit into from
Jan 6, 2026
Merged

refactor: handle INJECT_FACTS_AS_VARS=false by using ansible_facts instead #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
selectattr('previous', 'match', '^replaced$') |
list | length > 0 }}"
__firewall_python_cmd: "{{ ansible_python_interpreter |
d(discovered_interpreter_python) }}"
d(ansible_facts['discovered_interpreter_python']) }}"
Comment on lines 42 to +43
Copy link

@sourcery-ai sourcery-ai bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Indexing ansible_facts['discovered_interpreter_python'] directly may raise an error if the fact is unexpectedly missing.

Inside d(), this will fail outright if discovered_interpreter_python isn’t in ansible_facts. To make it more robust while preserving the new behavior, you could use:

__firewall_python_cmd: "{{ ansible_python_interpreter |
  d(ansible_facts.get('discovered_interpreter_python')) }}"
Suggested change
__firewall_python_cmd: "{{ ansible_python_interpreter |
d(discovered_interpreter_python) }}"
d(ansible_facts['discovered_interpreter_python']) }}"
__firewall_python_cmd: "{{ ansible_python_interpreter |
d(ansible_facts.get('discovered_interpreter_python')) }}"

__firewall_report_changed: true

- name: Handle previous replaced
Expand Down
24 changes: 12 additions & 12 deletions tests/tests_ansible.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,8 +614,8 @@
register: result
failed_when: result is failed or result is not changed
when:
- ansible_distribution in ["RedHat", "CentOS", "Fedora"]
- ansible_distribution_major_version is version("8", ">=")
- ansible_facts["distribution"] in ["RedHat", "CentOS", "Fedora"]
- ansible_facts["distribution_major_version"] is version("8", ">=")

- name: Add includes again to check idempotence
firewall_lib:
Expand All @@ -629,8 +629,8 @@
register: result
failed_when: result is failed or result is changed
when:
- ansible_distribution in ["RedHat", "CentOS", "Fedora"]
- ansible_distribution_major_version is version("8", ">=")
- ansible_facts["distribution"] in ["RedHat", "CentOS", "Fedora"]
- ansible_facts["distribution_major_version"] is version("8", ">=")

- name: Delete custom service
firewall_lib:
Expand Down Expand Up @@ -665,12 +665,12 @@
- "aaaa:aaaa:aaaa:aaa:aaaa:aaaa:aaaa::"
# these two don't exist yet in RHEL 7
helper_module: "{{ 'nf_conntrack_ftp'
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else omit }}"
includes: "{{ ['https', 'ldaps']
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else omit }}"
online: "{{ __firewall_is_booted }}"
register: result
Expand All @@ -693,8 +693,8 @@
that:
- '"includes: https ldaps" in info.stdout'
- '"helpers: nf_conntrack_ftp" in info.stdout'
when: ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
when: ansible_facts["distribution"] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts["distribution_major_version"] is version('8', '>=')

- name: Add helper module that already is on customservice
firewall_lib:
Expand All @@ -705,8 +705,8 @@
online: "{{ __firewall_is_booted }}"
register: result
failed_when: result is failed or result is changed
when: ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
when: ansible_facts["distribution"] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts["distribution_major_version"] is version('8', '>=')

- name: Forward port 40 to 0.0.0.0:8080 (string)
firewall_lib:
Expand Down
4 changes: 2 additions & 2 deletions tests/tests_interface_pci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
set_fact:
nftables_backend:
"{{ true
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else false }}"

- name: Find ethernet interface
Expand Down
4 changes: 2 additions & 2 deletions tests/tests_reload_on_reset.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
- name: Test not supported on EL7
meta: end_host
when:
- ansible_distribution in ['RedHat', 'CentOS']
- ansible_distribution_major_version | int < 8
- ansible_facts["distribution"] in ['RedHat', 'CentOS']
- ansible_facts["distribution_major_version"] | int < 8
tasks:
- name: Determine if system is ostree and set flag
when: not __firewall_is_ostree is defined
Expand Down
40 changes: 20 additions & 20 deletions tests/tests_service.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@
- 1.1.1.1
- 1::1
includes: "{{ ['ssh', 'ldaps']
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else [] }}"
permanent: true
state: present
Expand All @@ -93,8 +93,8 @@
- 1.1.1.1
- 1::1
includes: "{{ ['ssh', 'ldaps']
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else [] }}"
permanent: true
state: present
Expand Down Expand Up @@ -127,12 +127,12 @@
protocol: icmp
# these two don't exist yet in RHEL 7
helper_module: "{{ 'ftp'
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else omit }}"
includes: "{{ ['ssh', 'ldaps']
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else [] }}"
permanent: true
state: present
Expand Down Expand Up @@ -175,12 +175,12 @@
protocol: icmp
# these two don't exist yet in RHEL 7
helper_module: "{{ 'ftp'
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else omit }}"
includes: "{{ ['ssh', 'ldaps']
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else [] }}"
permanent: true
state: present
Expand Down Expand Up @@ -257,12 +257,12 @@
- 1::1
protocol: icmp
helper_module: "{{ 'ftp'
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else omit }}"
includes: "{{ ['ssh', 'ldaps']
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else [] }}"
permanent: true
state: absent
Expand All @@ -287,12 +287,12 @@
- 1::1
protocol: icmp
helper_module: "{{ 'ftp'
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else omit }}"
includes: "{{ ['ssh', 'ldaps']
if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
and ansible_distribution_major_version is version('8', '>=')
if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
and ansible_facts['distribution_major_version'] is version('8', '>=')
else [] }}"
permanent: true
state: absent
Expand Down
6 changes: 3 additions & 3 deletions tests/tests_zone.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
gather_facts: true
tasks:
- name: Test firewalld zones
when: ansible_distribution == "Fedora" or
((ansible_distribution == "CentOS" or ansible_distribution == "RedHat")
and ansible_distribution_major_version | int >= 7)
when: ansible_facts["distribution"] == "Fedora" or
((ansible_facts["distribution"] == "CentOS" or ansible_facts["distribution"] == "RedHat")
and ansible_facts["distribution_major_version"] | int >= 7)
block:

# INIT TEST
Expand Down
4 changes: 2 additions & 2 deletions tests/vars/rh_distros_vars.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ __firewall_rh_distros:
__firewall_rh_distros_fedora: "{{ __firewall_rh_distros + ['Fedora'] }}"

# Use this in conditionals to check if distro is Red Hat or clone
__firewall_is_rh_distro: "{{ ansible_distribution in __firewall_rh_distros }}"
__firewall_is_rh_distro: "{{ ansible_facts['distribution'] in __firewall_rh_distros }}"

# Use this in conditionals to check if distro is Red Hat or clone, or Fedora
__firewall_is_rh_distro_fedora: "{{ ansible_distribution in __firewall_rh_distros_fedora }}"
__firewall_is_rh_distro_fedora: "{{ ansible_facts['distribution'] in __firewall_rh_distros_fedora }}"
12 changes: 9 additions & 3 deletions vars/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,20 @@ __firewall_usr_lib_dir: /usr/lib/firewalld

# ansible_facts required by the role
__firewall_required_facts:
- discovered_interpreter_python
- distribution_major_version
- distribution
- python_version
- service_mgr

__firewall_no_subsets_facts:
- discovered_interpreter_python

# the subsets of ansible_facts that need to be gathered in case any of the
# facts in required_facts is missing; see the documentation of
# the 'gather_subset' parameter of the 'setup' module
__firewall_required_facts_subsets: "{{ ['!all', '!min'] +
__firewall_required_facts }}"
__firewall_required_facts | difference(__firewall_no_subsets_facts) }}"
Comment on lines +14 to +21
Copy link

@sourcery-ai sourcery-ai bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: The difference filter plus list concatenation relies on Jinja operator precedence; consider making it explicit for readability.

This expression depends on Jinja evaluating it as (['!all', '!min'] + __firewall_required_facts) | difference(...). To avoid ambiguity and make future changes safer, consider adding parentheses:

__firewall_required_facts_subsets: "{{ (['!all', '!min'] + __firewall_required_facts)
  | difference(__firewall_no_subsets_facts) }}"


__firewall_packages_base: [firewalld]

Expand Down Expand Up @@ -42,8 +48,8 @@ __firewall_rh_distros:
__firewall_rh_distros_fedora: "{{ __firewall_rh_distros + ['Fedora'] }}"

# Use this in conditionals to check if distro is Red Hat or clone
__firewall_is_rh_distro: "{{ ansible_distribution in __firewall_rh_distros }}"
__firewall_is_rh_distro: "{{ ansible_facts['distribution'] in __firewall_rh_distros }}"

# Use this in conditionals to check if distro is Red Hat or clone, or Fedora
__firewall_is_rh_distro_fedora: "{{ ansible_distribution in __firewall_rh_distros_fedora }}"
__firewall_is_rh_distro_fedora: "{{ ansible_facts['distribution'] in __firewall_rh_distros_fedora }}"
# END - DO NOT EDIT THIS BLOCK - rh distros variables
Loading