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

Normalize extra_vars support for Ansible Tower playbooks. #18057

Merged
merged 2 commits into from
Oct 18, 2018

Conversation

lfu
Copy link
Member

@lfu lfu commented Oct 4, 2018

  • Refactor the support of extra_vars for ServiceAnsiblePalybook and automate playbook method.
  • Add extra_vars support for ServiceAnsibleTower.

Includes ManageIQ/manageiq-automation_engine#244

Fixes #18005.

@miq-bot assign @gmcculloug
@miq-bot add_label enhancement, services

cc @mkanoor

@lfu lfu changed the title Service ansible mixin Normalize extra_vars support for Ansible Tower playbooks. Oct 4, 2018
@gmcculloug
Copy link
Member

@lfu @mkanoor Should these changes be linked to https://bugzilla.redhat.com/show_bug.cgi?id=1611946

Can we solve the automate_workspace issue from that ticket (see https://bugzilla.redhat.com/show_bug.cgi?id=1611946#c16) with these changes?

@syncrou
Copy link
Contributor

syncrou commented Oct 4, 2018

@lfu - Regarding @gmcculloug's comment it seems there would need to be the addition of the automate_workspace key in the manageiq_connection hash to solve that issue.

@@ -1,4 +1,5 @@
class ServiceAnsibleTower < Service
include ApiAnsibleMixin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lfu should this file name indicate that its ExtraVars maybe ApiExtraVars. Since most of the fields in there are going in as ExtraVars for connecting to our REST API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not good at naming. But I think ApiAnsibleMixin makes more sense than ApiExtraVarsMixin in terms of audience. People may have heard of ansible but not extra_vars.

@gmcculloug Ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I'm helping here, but I came up with: AnsibleCallbackExtraVarsMixin

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the module to AnsibleExtraVarsMixin.

@lfu lfu force-pushed the service_ansible_mixin branch from 4358c88 to 37fdd6c Compare October 4, 2018 19:42
@lfu
Copy link
Member Author

lfu commented Oct 4, 2018

@gmcculloug @syncrou Regarding ServiceAnsiblePalybook, this PR only refactor the existing code for extra_vars which won't fix the issue in https://bugzilla.redhat.com/show_bug.cgi?id=1611946.

@lfu
Copy link
Member Author

lfu commented Oct 10, 2018

Tested with Ansible Tower Service, Ansible Playbook Service and automate playbook method. manageiq_env and manageiq_connection_env hashes were added into playbooks' extra_vars.

@mkanoor
Copy link
Contributor

mkanoor commented Oct 12, 2018

@lfu If you have examples in your playbook repo can you post the link here.
It might make sense to put them in a different directory since it can be shared across

  • Ansible Tower Service
  • Ansible Playbook Service
  • Ansible Playbook Method

@lfu
Copy link
Member Author

lfu commented Oct 15, 2018

The playbook I used for testing is this one.

@lfu lfu force-pushed the service_ansible_mixin branch from 993ba89 to 3b6fa05 Compare October 15, 2018 21:58
@miq-bot
Copy link
Member

miq-bot commented Oct 15, 2018

Checked commits lfu/manageiq@b9ccc09~...3b6fa05 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 2 offenses detected

app/models/service_ansible_playbook.rb

app/models/service_ansible_tower.rb

@gmcculloug gmcculloug merged commit 9b265d4 into ManageIQ:master Oct 18, 2018
@gmcculloug gmcculloug added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 18, 2018
lfu added a commit to lfu/manageiq-content that referenced this pull request Oct 22, 2018
lfu added a commit to lfu/manageiq that referenced this pull request Oct 22, 2018
@lfu lfu deleted the service_ansible_mixin branch July 29, 2019 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants