Add Ansible diff mode #98
Conversation
adamruzicka
left a comment
adamruzicka
left a comment
There was a problem hiding this comment.
At first glance this looks good, waiting for the followup PRs in related repos.
pr-processor
bot
added
Needs re-review
and removed
Not yet reviewed
Needs re-review
labels
|
Next PR is theforeman/foreman-ansible-modules#1816 |
|
Further pull requests: theforeman/foreman_ansible#749 |
|
Any update? |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
I wonder if it makes sense to expose this as a capability. In https://theforeman.org/2019/04/smart-proxy-capabilities-explained.html I showed how that can be done. Then on the Foreman side you can know if the Smart Proxy supports diff mode and can error out if it was requested, but not supported. Or at least show a warning.
| @rex_command = action_input[:remote_execution_command] | ||
| @check_mode = action_input[:check_mode] | ||
| @job_check_mode = action_input[:job_check_mode] | ||
| @diff_mode = action_input[:diff_mode] |
There was a problem hiding this comment.
We could, but this would require changes to theforeman/foreman_ansible#749 as well similar to theforeman/foreman_ansible@76a53e3 I guess. Let me know what you prefer.
There was a problem hiding this comment.
We could, but this would require changes to theforeman/foreman_ansible#749 as well similar to theforeman/foreman_ansible@76a53e3 I guess. Let me know what you prefer.
Looking at theforeman/foreman_ansible#764 and #100 I think no changes are needed here and we just need to merge these and mine. There are some text conflicts but these should be easy to fix in a rebase of whatever PR is merged later.
There was a problem hiding this comment.
@bnerickson has submitted theforeman/foreman_ansible#764. I wouldn't mind merging this PR together with your foreman_ansible PR. @bnerickson would you be OK to rebase your work on top of this?
There was a problem hiding this comment.
I have no problem rebasing my work on top of this PR.
I looked at https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_checkmode.html and I wonder how bad it is if the Smart Proxy doesn't support diff mode. It may be frustrating, but I don't think it would cause harm (unlike if |
If you have a Foreman with theforeman/foreman_ansible#749 and a smart proxy without this PR it has no further impact but that the diffs are not displayed for Ansible jobs that have been executed by that smart proxy. As you state, it is just frustrating, but it does not cause harm. |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
👍 on the code, but could you rebase this and squash it into a single PR? We also require a Redmine issue. https://projects.theforeman.org/projects/ansible/issues/new can be used to create one.
Add the possibility to turn on Ansible diff mode by the new input parameter diff_mode. Signed-off-by: Ruediger Pluem <[email protected]> Update lib/smart_proxy_ansible/runner/ansible_runner.rb Co-authored-by: Adam Růžička <[email protected]>
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
I approved the CI run. Perhaps @adamruzicka wants to weigh in too, otherwise I'll merge this soon
adamruzicka
left a comment
adamruzicka
left a comment
There was a problem hiding this comment.
This looks reasonable
4 participants
Add the possibility to turn on Ansible diff mode by the new input parameter
diff_mode.There will be further PR's following to https://github.com/theforeman/foreman-ansible-modules and https://github.com/theforeman/foreman_ansible to enable this on the Foreman server and make use of it in the configuration reports.