fixes #20407 - adds two new options #94

mriedel-pgdx · 2017-07-25T13:44:36Z

Fixes #20407 - adds two new options.
Adds ability to configure ansible_become & ansible_ssh_private_key_file options to foreman_ansible plugin. IDE also edited whitespace in ansible.rb file.

theforeman-bot · 2017-07-25T13:44:36Z

There were the following issues with the commit message:

c88ad5b must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-07-25T13:47:25Z

There were the following issues with the commit message:

length of the first commit message line for 83d1b19 exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

xprazak2 · 2017-07-27T10:09:47Z

app/models/setting/ansible.rb

+              'ansible_ssh_private_key_file',
+              N_('Use this to supply a path to an SSH Private Key '\
+                 'that Ansible will use in lieu of a password '\
+                 'Override with "ansible_ssh_private_key_file"'\


nit: needs a space after the closing "

xprazak2 · 2017-07-27T10:11:03Z

app/models/setting/ansible.rb

+              N_('Foreman will use the sudo command to run roles on hosts '\
+                 'You can override this on hosts by adding a parameter '\
+                 '"ansible_become"'),
+              'True',


try replacing 'True' with true, our helper should recognize the boolean and use dropdropdown instead of text field

xprazak2 · 2017-07-27T10:19:51Z

app/services/foreman_ansible/inventory_creator.rb

+      # Check if private_key is defined, if it is use that. Otherwise
+      # use ssh_pass
+      if !host.host_params['ansible_ssh_private_key_file'].empty? ||
+          !Setting[:private_key].empty?


I think this should be Setting[:ansible_ssh_private_key_file]. Note that condition will be true out of the box since you set the default value to /tmp/private_key_file, so I would recommend keeping it blank. You can also take advantage of your host_private_key_file method and change it to:

unless host_private_key_file(host).empty? # ... else # ... end

xprazak2 · 2017-07-27T10:25:50Z

[test]

xprazak2 · 2017-07-27T10:28:44Z

Thank you for your patch, I left a few comments inline, but it looks very nice!

- Fixes formatting - Fixes errant use of 'private_key' dictionary key - Changes boolean to 'true' - Removes default value for key path

mriedel-pgdx · 2017-07-27T12:38:08Z

Hi @xprazak2 ! Thanks for the feedback, and catching the 'private_key' I missed!

I've made the changes you suggested, except the "unless" because I'm using an "else" with it. My IDE raised a warning on that, and evidently there's some controversy over doing things that way.

Also, maybe this is a Foreman bug, but in my testing empty strings ('') as defaults caused the setting to disappear from the Ansible settings page in Foreman. The existing Default verbosity level setting does not appear in Foreman 1.15.2, and the same thing happened with ansible_ssh_private_key_file. Regardless, I've updated that too.

I'm attaching a screenshot of the missing "Verbosity" setting. This also happened with "Private Key Path" was set to empty.

- Realized I quoted the boolean.

xprazak2 · 2017-07-28T08:06:14Z

app/services/foreman_ansible/inventory_creator.rb

+      # Check if private_key is defined, if it is use that. Otherwise
+      # use ssh_pass
+      if !host.host_params['ansible_ssh_private_key_file'].empty? ||
+          Setting[:ansible_ssh_private_key_file].empty?


I think the ! got lost, it was here before. You can keep if, I do not mind either way. My main point was that you can re-use the logic from host_private_key_file method and be more DRY:

if !host_private_key_file(host).empty?

xprazak2 · 2017-07-28T08:16:18Z

I solved the missing setting mystery. We have to add a setting with a blank default into whitelist. If you add:

Setting::BLANK_ATTRS.push('ansible_ssh_private_key_file')

the setting will turn up. Here is an example how we use it in other places.

dLobatog

Thanks for the patch @mriedel-pgdx and @xprazak2 for the review. I'm cooking up something to follow this one up so that any ansible_ parameter gets converted into an option so that we don't need to add these kind of options anymore. #95

After that's done and this one is merged I think it's time for a release 😉

- Taking educated guess on where to put the BLANK_ATTRS push - Using 'host_private_key_file' check in an DRY attempt.

dLobatog

@mriedel-pgdx Sorry, this requires rebasing now. Basically you should feel free to remove the host_become and host_private_key_file methods and add these settings to the ansible_settings method. I'll merge after that if tests pass. Thanks!

- Removes host_become & host_private_key_file - Left settings in Ansible Settings module - Reverted ansible_ssh_pass to default parameter (as it was prior to pull request)

theforeman-bot · 2017-08-01T12:46:49Z

There were the following issues with the commit message:

f1b0c3e must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

mriedel-pgdx · 2017-08-01T12:48:19Z

@dLobatog Thanks! I merged the changes in and recommitted.

dLobatog · 2017-08-01T14:38:08Z

Thanks @mriedel-pgdx! Note you can use any arbitrary ansible CLI option using the latest Foreman Ansible version (only available in git now, I will release a package next week probably).

Basically any ansible_ param will be passed on to the playbook so it can be used as a connection option, similar to what you did here, except other parameters won't have a global setting.

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Jul 25, 2017

mriedel-pgdx force-pushed the master branch from c88ad5b to 83d1b19 Compare July 25, 2017 13:47

theforeman-bot added Needs testing Waiting on contributor and removed Waiting on contributor labels Jul 25, 2017

fixes #20407 - Added two options

e5a0f5a

mriedel-pgdx force-pushed the master branch from 83d1b19 to e5a0f5a Compare July 25, 2017 13:48

theforeman-bot added Needs testing and removed Waiting on contributor labels Jul 25, 2017

mriedel-pgdx added 5 commits July 25, 2017 10:02

fixes #20407 - updating formatting

6475d2e

fixes #20407 - updating formatting

5baaa48

fixes #20407 - updating formatting

16c6545

fixes #20407 - updating formatting

2117f30

fixes #20407 - updating formatting

a6c407c

xprazak2 reviewed Jul 27, 2017

View reviewed changes

fixes #20407 - incorporates feedback from pull request

c7785b7

- Fixes formatting - Fixes errant use of 'private_key' dictionary key - Changes boolean to 'true' - Removes default value for key path

fixes #20407 - incorporates feedback from pull request

a580028

- Realized I quoted the boolean.

xprazak2 reviewed Jul 28, 2017

View reviewed changes

dLobatog suggested changes Jul 31, 2017

View reviewed changes

theforeman-bot removed the Not yet reviewed label Jul 31, 2017

theforeman-bot added the Waiting on contributor label Jul 31, 2017

fixes #20407 - incorporates feedback from pull request

e6ebeeb

- Taking educated guess on where to put the BLANK_ATTRS push - Using 'host_private_key_file' check in an DRY attempt.

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jul 31, 2017

dLobatog suggested changes Aug 1, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Aug 1, 2017

fixes #20407 - incorporates feedback from pull request

7619263

- Removes host_become & host_private_key_file - Left settings in Ansible Settings module - Reverted ansible_ssh_pass to default parameter (as it was prior to pull request)

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Aug 1, 2017

Merge branch 'master' into master

f1b0c3e

theforeman-bot added the Waiting on contributor label Aug 1, 2017

dLobatog approved these changes Aug 1, 2017

View reviewed changes

dLobatog merged commit e60bf28 into theforeman:master Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #20407 - adds two new options #94

fixes #20407 - adds two new options #94

mriedel-pgdx commented Jul 25, 2017 •

edited

theforeman-bot commented Jul 25, 2017

theforeman-bot commented Jul 25, 2017

xprazak2 Jul 27, 2017

xprazak2 Jul 27, 2017

xprazak2 Jul 27, 2017

xprazak2 commented Jul 27, 2017

xprazak2 commented Jul 27, 2017

mriedel-pgdx commented Jul 27, 2017 •

edited

xprazak2 Jul 28, 2017

xprazak2 commented Jul 28, 2017

dLobatog left a comment •

edited

dLobatog left a comment

theforeman-bot commented Aug 1, 2017

mriedel-pgdx commented Aug 1, 2017

dLobatog commented Aug 1, 2017

fixes #20407 - adds two new options #94

fixes #20407 - adds two new options #94

Conversation

mriedel-pgdx commented Jul 25, 2017 • edited

theforeman-bot commented Jul 25, 2017

theforeman-bot commented Jul 25, 2017

xprazak2 Jul 27, 2017

Choose a reason for hiding this comment

xprazak2 Jul 27, 2017

Choose a reason for hiding this comment

xprazak2 Jul 27, 2017

Choose a reason for hiding this comment

xprazak2 commented Jul 27, 2017

xprazak2 commented Jul 27, 2017

mriedel-pgdx commented Jul 27, 2017 • edited

xprazak2 Jul 28, 2017

Choose a reason for hiding this comment

xprazak2 commented Jul 28, 2017

dLobatog left a comment • edited

Choose a reason for hiding this comment

dLobatog left a comment

Choose a reason for hiding this comment

theforeman-bot commented Aug 1, 2017

mriedel-pgdx commented Aug 1, 2017

dLobatog commented Aug 1, 2017

mriedel-pgdx commented Jul 25, 2017 •

edited

mriedel-pgdx commented Jul 27, 2017 •

edited

dLobatog left a comment •

edited