-
Notifications
You must be signed in to change notification settings - Fork 112
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
Fixes #37942 - Adding Ansible Check Mode to Ansible Job Templates #742
base: master
Are you sure you want to change the base?
Conversation
64bb115
to
9e70d02
Compare
9e70d02
to
3e01dae
Compare
3e01dae
to
614c4f2
Compare
Would it also make sense to have this on the job level? |
Agreed! It'd be great if this were also available on a job-based level. I might tackle that after job templates are working. I don't know where to start there yet. |
cc @Thorben-D |
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.
I'm currently facing some issues with my local environment, so I'll need to delay testing until they're resolved. In the meantime, to answer your question:
For the db/migrate/20241022000000_add_ansible_check_mode_to_templates.rb file, I arbitrarily set the date string in the filename to 20241022000000, but is that supposed to be automatically generated somehow?
Yes. If you run the Rails generator command to create a migration (rails generate migration
), Rails automatically prefixes the migration file name with the appropriate timestamp.
As long as the timestamp doesn't conflict with existing migration timestamps, it's fine to proceed with the manually set value.
@@ -0,0 +1,10 @@ | |||
# frozen_string_literal: true |
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.
This is no longer needed. See:
foreman_ansible/.rubocop_todo.yml
Lines 160 to 161 in a6f5ac9
Style/FrozenStringLiteralComment: | |
Enabled: false |
@@ -0,0 +1,10 @@ | |||
# frozen_string_literal: true | |||
|
|||
class AddAnsibleCheckModeToTemplates < ActiveRecord::Migration[6.0] |
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.
Could you update the Rails version to 7.0 or higher?
RemoteExecutionFeature.where(label: 'ansible_run_host').each do |rex_feature| | ||
Template.where(id: rex_feature.job_template_id).update_all(ansible_check_mode: false) | ||
end |
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.
Since the default value is already set to false
, there's no need to update these templates again.
Overview of Changes
This PR intends to add the Ansible Check Mode (identified with the
--check
argument to ansible-runner) to Ansible Job Templates via a checkbox option. The follow-up after all the subsequent PRs are submitted/approved (hammer cli support for instance) is to add Ansible Diff Mode support in a similar manner. See below screenshot:Implementation Considerations
Foreman already offers the ability for users to run Ansible Config Commands (Running Ansible Roles on a host) with check mode enabled by setting the host's host parameter ansible_roles_check_mode to True. This is left in-place as-is. However, that implementation does not allow users to run Ansible Playbook jobs or any other custom remote execution jobs in Ansible Check Mode. Nor is setting a host parameter as flexible as setting an entire Job Template to use Ansible Check Mode.
I have a few questions/concerns:
Testing Steps
Checklists
Additional Notes
This PR is not dependent on any other changes. The follow PRs are dependent on this change: