Skip to content
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

fix: Run Ansible versions validation tasks on check mode #841

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

Al-thi
Copy link
Contributor

@Al-thi Al-thi commented Dec 19, 2024

Proposed changes

fix #840

Since a6712e3, ansible and jinja versions are validated to ensure supported versions are used.
This validation is done by delegating command tasks to localhost, and parsing the standard output of the executed commands, e.g. ansible --version.

The ansible.builtin.command module is not run when in check mode, causing the variable which is supposed to get the result of the command to be empty, resulting in an error message in the following tasks that parse these result variables.

This commit ensures the command tasks are run even in check_mode. As they do no modification on localhost, this is not dangerous.

Checklist

Before creating a PR, run through this checklist and mark each as complete:

@Al-thi Al-thi requested a review from alessfg as a code owner December 19, 2024 08:44
Copy link

github-actions bot commented Dec 19, 2024

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@github-actions github-actions bot added the bug Something isn't working label Dec 19, 2024
@Al-thi Al-thi force-pushed the fix/versions-validation branch from 3c50a6e to cab6adb Compare December 19, 2024 08:45
@Al-thi
Copy link
Contributor Author

Al-thi commented Dec 19, 2024

I have hereby read the F5 CLA and agree to its terms

@alessfg
Copy link
Collaborator

alessfg commented Jan 8, 2025

Thanks for the PR! I feel like we should probably leave both the entire jinja2 and collections checks out of check mode? There's not much to assert in the assert task if we skip the tasks to the get the info we need to make the assertions.

@Al-thi
Copy link
Contributor Author

Al-thi commented Jan 8, 2025

Thanks for the PR! I feel like we should probably leave both the entire jinja2 and collections checks out of check mode? There's not much to assert in the assert task if we skip the tasks to the get the info we need to make the assertions.

Mmmm i'm not sure about this. My commit is not to skip the validation tasks, but to run them correctly even in check-mode. Is this not what is best ? Thank you

@Al-thi Al-thi force-pushed the fix/versions-validation branch from cab6adb to 3c43ff3 Compare January 8, 2025 15:41
@alessfg
Copy link
Collaborator

alessfg commented Jan 10, 2025

You are absolutely right! I glanced over the PR first thing in the morning before I caffeinated myself and I totally misread the text. One last thing, can you update the changelog?

@alessfg alessfg changed the title fix: ansible versions validation tasks fix: Run Ansible versions validation tasks on check mode Jan 10, 2025
@alessfg alessfg added this to the 0.25.1 milestone Jan 10, 2025
fix nginxinc#840

Since a6712e3, ansible and jinja versions are
validated to ensure supported versions are used.
This validation is done by delegating `command` tasks to localhost, and parsing
the standard output of the executed commands, e.g. `ansible --version`.

The `ansible.builtin.command` module is not run when in check mode, causing the
variable which is supposed to get the result of the command to be empty,
resulting in an error message in the following tasks that parse these result
variables.

This commit ensures the `command` tasks are run even in check_mode. As they do
no modification on localhost, this is not dangerous.

Signed-off-by: Alexis Thietard <[email protected]>
@Al-thi Al-thi force-pushed the fix/versions-validation branch from 3c43ff3 to 08f9969 Compare January 10, 2025 13:31
@Al-thi
Copy link
Contributor Author

Al-thi commented Jan 10, 2025

You are absolutely right! I glanced over the PR first thing in the morning before I caffeinated myself and I totally misread the text.

Haha no problem, I know the feeling.

One last thing, can you update the changelog?

Done, I hope that's OK :)

@alessfg alessfg enabled auto-merge January 10, 2025 13:58
@alessfg alessfg added this pull request to the merge queue Jan 10, 2025
Merged via the queue into nginxinc:main with commit 87e3ff2 Jan 10, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

Check mode is broken by ansible version validation
2 participants