-
Couldn't load subscription status.
- Fork 11
fix: Fixes for Ansible 2.19 #129
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
Cause: The test used a temporary variable `ansible_managed`, but that is a "magic" string constant. Ansible 2.19 does not permit assigning to it any more. Consequence: Tests failed with Ansible 2.19. Fix: Rename the variable.
Cause: `systemctl is-system-running` was not called in `--check` mode. Consequence: The "Determine if system is booted with systemd" step failed in check mode as the `__is_system_running` variable was not populated. Fix: Force calling `systemctl is-system-running` in check mode. It does not modify the system and the outcome is very influential for what the role does.
Reviewer's GuideEnsures Ansible 2.19 compatibility by updating the managed header variable reference and disabling check_mode on the systemctl health check task. Sequence Diagram: Impact of 'check_mode: false' on 'systemctl is-system-running' TasksequenceDiagram
participant Ansible
participant SystemctlTask as "'systemctl is-system-running' Task\n(with check_mode: false)"
participant Systemd
Ansible->>SystemctlTask: Execute Task (even if Ansible is in global check_mode)
SystemctlTask->>Systemd: systemctl is-system-running (actual execution)
Systemd-->>SystemctlTask: Actual status
SystemctlTask->>Ansible: Register __is_system_running
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
OK, necessary but not sufficient, but we can land the fixes in steps. |
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 @martinpitt - I've reviewed your changes - here's some feedback:
- Consider supporting both
ansible_managedand__ansible_managedin the header test so it still passes on older Ansible versions. - Instead of unconditionally disabling
check_mode, consider wrapping that option in a version check (e.g.when: ansible_version.full is version('2.19', '>=')) or adding a comment explaining why it’s needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider supporting both `ansible_managed` and `__ansible_managed` in the header test so it still passes on older Ansible versions.
- Instead of unconditionally disabling `check_mode`, consider wrapping that option in a version check (e.g. `when: ansible_version.full is version('2.19', '>=')`) or adding a comment explaining why it’s needed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Fixes for the f42/ansible 2.19 failures introduced in #128. This isn't complete yet, so starting with draft.
Summary by Sourcery
Fix failures under Ansible 2.19 by correcting variable references in header assertions and ensuring the systemd status check runs outside of check mode.
Bug Fixes: