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.

Sign up for GitHub

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

Jump to bottom

Cleanup after Ansible runner integration #594

Merged
merged 1 commit into from
Oct 31, 2019
Merged

Cleanup after Ansible runner integration #594

merged 1 commit into from
Oct 31, 2019

Conversation

lfu
Copy link
Member

@lfu lfu commented Oct 28, 2019 •
edited
Loading

Depends on ManageIQ/manageiq-automation_engine#383.
Related to ManageIQ/manageiq#19383.

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

@miq-bot add_label refactoring, changelog/yes, ivanchuk/yes, hammer/no, embedded ansible, services

@lfu lfu mentioned this pull request Oct 28, 2019
Merged
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3771

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 96.797%
Totals Coverage Status
Change from base Build 3756: -0.005%
Covered Lines: 2750
Relevant Lines: 2841
💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 28, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 3775

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.006%) to 96.796%
Totals Coverage Status
Change from base Build 3756: -0.006%
Covered Lines: 2749
Relevant Lines: 2840
💛 - Coveralls

@lfu lfu force-pushed the ansible_runner_cleanup_credential branch 2 times, most recently from e975089 to c7e4092 Compare October 29, 2019 15:25
@lfu lfu mentioned this pull request Oct 29, 2019
Merged
gmcculloug
gmcculloug reviewed Oct 30, 2019
View reviewed changes
...omationManagement/AnsibleTower/Operations/Methods.class/__methods__/available_credentials.rb
@@ -25,7 +25,7 @@ def provider
# for reconfig and retire use service
# Or use the embedded_ansible_provider
service = @handle.root['service_template'] || @handle.root['service']
service.try(:job_template, 'Provision').try(:manager) || embedded_ansible_provider
service.try(:playbook, 'Provision').try(:manager) || embedded_ansible_provider
Copy link
Member

Choose a reason for hiding this comment

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

@lfu I have come around to agreeing with you on this change after a bit more searching. The only model that supported a job_template method that also accepts a parameter was ServiceTemplateAnsiblePlaybook

With the changes to support Runner and the related refactorings, that method was renamed to playbook. See https://github.com/ManageIQ/manageiq/blob/master/app/models/service_template_ansible_playbook.rb#L75

I also checked the code base at the time this available_credentials.rb script was initially created git checkout 'master@{2017-02-16}' and see that ServiceTemplateAnsiblePlaybook was the only model to support job_template with a parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

...onManagement/AnsibleTower/Operations/Methods.class/__methods__/available_credentials_spec.rb Outdated
@@ -2,8 +2,8 @@

describe ManageIQ::Automate::AutomationManagement::AnsibleTower::Operations::AvailableCredentials do
let(:ansible_manager) { FactoryBot.create(:automation_manager_ansible_tower) }
Copy link
Member

Choose a reason for hiding this comment

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

@lfu Should this now be embedded_automation_manager_ansible or provider_embedded_ansible?

@lfu lfu force-pushed the ansible_runner_cleanup_credential branch from c7e4092 to f424d8a Compare October 30, 2019 21:00
@miq-bot
Copy link
Member

miq-bot commented Oct 30, 2019

Checked commit lfu@f424d8a with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@gmcculloug gmcculloug self-assigned this Oct 30, 2019
@tinaafitz tinaafitz merged commit 61527d2 into ManageIQ:master Oct 31, 2019
@tinaafitz tinaafitz added this to the Sprint 124 Ending Nov 11, 2019 milestone Oct 31, 2019
@lfu lfu deleted the ansible_runner_cleanup_credential branch November 4, 2019 15:27
simaishi pushed a commit that referenced this pull request Dec 17, 2019
@tinaafitz @simaishi
Merge pull request #594 from lfu/ansible_runner_cleanup_credential
f20f667 
Cleanup after Ansible runner integration

(cherry picked from commit 61527d2)

https://bugzilla.redhat.com/show_bug.cgi?id=1784103
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit f20f66725193b2647a34235537de31af468584e5
Author: tina <tfitzger@redhat.com>
Date:   Thu Oct 31 10:09:30 2019 -0400

    Merge pull request #594 from lfu/ansible_runner_cleanup_credential

    Cleanup after Ansible runner integration

    (cherry picked from commit 61527d22c871b56b969fcb6027acccc70439c53c)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug left review comments

@tinaafitz tinaafitz tinaafitz approved these changes

@mkanoor mkanoor mkanoor approved these changes

Assignees

@gmcculloug gmcculloug

Labels
changelog/yes embedded ansible hammer/no ivanchuk/backported refactoring services
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lfu @coveralls @miq-bot @simaishi @tinaafitz @gmcculloug @mkanoor