Alter ansible_playbook method so that some arguments are optional

[djberg96]

This PR modifies the ansible_playbook method of the ConversionHost model so that the 2nd and 3rd arguments are optional. The extra_vars will simply default to an empty hash, while the local_connection argument (formerly just connection) is now a boolean argument that defaults to false.

I also added some comments.

This is a nicer approach, since the 3rd argument currently isn't used at all unless it's set to 'local', so it made sense to me to make it a boolean with a default value. Giving the 2nd argument a default value means we don't have to explicitly pass an empty hash when no extra vars are needed.

This PR also solves bugs with several internal methods where the 3rd argument is missing, e.g. check_conversion_host_role.

https://bugzilla.redhat.com/show_bug.cgi?id=1622728
https://bugzilla.redhat.com/show_bug.cgi?id=1695767

[djberg96]

@fdupont-redhat Please take a look.

[ghost]

Totally make sense. 👍

[djberg96]

@miq-bot add_reviewer @agrare

[agrare]

@djberg96 @fdupont-redhat I don't see any of the callers using the local_connection option, is there a reason to keep it?

I'm okay with this but would rather see us moving to our standard ansible-runner (https://github.com/ManageIQ/manageiq/blob/master/lib/ansible/runner.rb) instead of carrying this ansible_playbook method in ConversionHost

[djberg96]

@agrare Good question, I figured it was for debugging, but maybe that can happen with a local config file? @fdupont-redhat Is that the case?

As for using Ansible::Runner, how would we run it remotely?

[agrare]

As for using Ansible::Runner, how would we run it remotely?

We would have to add inventory (e.g. https://github.com/ansible/ansible-runner/blob/master/demo/inventory/hosts) since right now we've only run playbooks locally but it is an enhancement we've wanted to have so this would be a great time to add it.

[djberg96]

@agrare Ok, but let's just get it working for now, and I (or someone) will create a separate refactoring PR that uses our ansible runner lib.

[agrare]

@djberg96 oh yeah I wasn't suggesting to do that in this PR, the extra local connection was the only comment re this PR

[agrare]

@fdupont-redhat waiting on you to get back to us about if we can delete the local connection option

[ghost]

I'm ok to remove local connection option.

[djberg96]

@fdupont-redhat @agrare Ok, I've removed that option and the related comments.

[miq-bot]

Checked commits https://github.com/djberg96/manageiq/compare/31eeb2d67b3215cfdc65ac1b020dc161dcdd09b0~...64408d9cbfad3a80c7229de1bdf4c81b7fffeff6 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

[djberg96]

@miq-bot add_label hammer/yes

[simaishi]

Hammer backport details:

$ git log -1
commit 135db80dbb8e7559fbf32fe74750d017f677d1f4
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Jan 17 09:05:16 2019 -0500

    Merge pull request #18323 from djberg96/conversion_host_ansible_playbook
    
    Alter ansible_playbook method so that some arguments are optional
    
    (cherry picked from commit 658581253e0b061eb3266e5d2030b6f391d08ea6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1694229

[simaishi]

Reverted the backport:

commit c6bd093876a0a79240c83b9713fa380dee85af26
Author: Satoe Imaishi <simaishi@redhat.com>
Date:   Mon Apr 1 11:30:25 2019 -0400

    Revert "Merge pull request #18323 from djberg96/conversion_host_ansible_playbook"
    
    This reverts commit 135db80dbb8e7559fbf32fe74750d017f677d1f4.

[simaishi]

Hammer backport details:

$ git log -1
commit 3f30559bc27438bf078fbcab866bd5473f6bcee2
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Jan 17 09:05:16 2019 -0500

    Merge pull request #18323 from djberg96/conversion_host_ansible_playbook
    
    Alter ansible_playbook method so that some arguments are optional
    
    (cherry picked from commit 658581253e0b061eb3266e5d2030b6f391d08ea6)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1702020