-
Notifications
You must be signed in to change notification settings - Fork 45
refactor: handle INJECT_FACTS_AS_VARS=false by using ansible_facts instead #317
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. 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 <[email protected]>
Reviewer's GuideRefactors Ansible role and test playbooks to stop using deprecated fact-as-variable shortcuts and instead consistently reference data via ansible_facts, with a few related quoting hardenings in shell/command invocations. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[citest] |
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 found 1 issue, and left some high level feedback:
- Now that everything is using
ansible_facts[...], consider switching to dot-notation (e.g.ansible_facts.default_ipv4.interface) for readability and to match common Ansible style, as it’s equivalent here. - You have several repeated inline distro checks using
ansible_facts['distribution']/distribution_version; where possible, consider reusing the existing helper vars like__timesync_is_rh_distro(_fedora)to keep the logic centralized and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that everything is using `ansible_facts[...]`, consider switching to dot-notation (e.g. `ansible_facts.default_ipv4.interface`) for readability and to match common Ansible style, as it’s equivalent here.
- You have several repeated inline distro checks using `ansible_facts['distribution']` / `distribution_version`; where possible, consider reusing the existing helper vars like `__timesync_is_rh_distro(_fedora)` to keep the logic centralized and easier to maintain.
## Individual Comments
### Comment 1
<location> `tasks/main.yml:141` </location>
<code_context>
- name: Run phc_ctl on PTP interface
- command: phc_ctl -q {{ timesync_ptp_domains[0].interfaces[0] }}
+ command: phc_ctl -q {{ timesync_ptp_domains[0].interfaces[0] | quote }}
register: timesync_phc_ctl_output
changed_when: false
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `quote` with the `command` module will pass literal quotes to `phc_ctl`, likely breaking the argument.
Because `command` does not use a shell, it passes arguments as-is after splitting on whitespace. Using `| quote` turns `eth0` into `'eth0'`, including the single quotes, which `phc_ctl` is unlikely to accept and may cause failures.
If you just want to safely pass the interface, keep `command` and remove `| quote`, e.g.:
```yaml
action: phc_ctl -q "{{ timesync_ptp_domains[0].interfaces[0] }}"
```
Only use `| quote` if you switch to the `shell` module and actually need shell-escaping semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| - name: Run phc_ctl on PTP interface | ||
| command: phc_ctl -q {{ timesync_ptp_domains[0].interfaces[0] }} | ||
| command: phc_ctl -q {{ timesync_ptp_domains[0].interfaces[0] | quote }} |
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.
issue (bug_risk): Using quote with the command module will pass literal quotes to phc_ctl, likely breaking the argument.
Because command does not use a shell, it passes arguments as-is after splitting on whitespace. Using | quote turns eth0 into 'eth0', including the single quotes, which phc_ctl is unlikely to accept and may cause failures.
If you just want to safely pass the interface, keep command and remove | quote, e.g.:
action: phc_ctl -q "{{ timesync_ptp_domains[0].interfaces[0] }}"Only use | quote if you switch to the shell module and actually need shell-escaping semantics.
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 [email protected]
Summary by Sourcery
Update Ansible role and tests to use ansible_facts-based lookups instead of legacy fact variables to comply with INJECT_FACTS_AS_VARS=false and upcoming Ansible deprecations.
Enhancements: