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

Service Playbook updates fqname and configuration_template #15007

Merged
merged 4 commits into from
May 8, 2017

Conversation

bzwei
Copy link
Contributor

@bzwei bzwei commented May 4, 2017

Multiple problems were discovered while debugging https://bugzilla.redhat.com/show_bug.cgi?id=1447701

  1. Fix update process to include :fqname for resource_actions
  2. Embedded job template, aka configuration_template, should not be recorded in options[:config_info]
  3. Method #create_dialogs can be used both by create_catalog_item and update_catalog_item

@bzwei
Copy link
Contributor Author

bzwei commented May 4, 2017

@miq-bot add_label bug, providers/ansible_tower, services, fine/yes, blocker
@miq-bot assign @gmcculloug
cc @syncrou @jntullo

@syncrou
Copy link
Contributor

syncrou commented May 5, 2017

@bzwei - I was testing this with the following scenario:

  1. Create new Ansible Playbook catalog item with no retirement playbook.
  2. Edit the catalog item and add a new retirement playbook.
  3. Edit the catalog item and remove the retirement playbook.
  4. Delete the entire catalog item.

With part 4 I ran into this error:

screen shot 2017-05-05 at 11 56 38 am

I'm not entirely sure the error came from your changes - but I believe it failed here. I'd be interested if you can duplicate it on your environment.

@bzwei
Copy link
Contributor Author

bzwei commented May 5, 2017

@syncrou please test with latest commit.

@@ -132,7 +132,7 @@ def self.validate_config_info(info)


def job_template(action)
resource_actions.find_by(:action => action.to_s.capitalize).try(:configuration_template)
resource_actions.find_by(:action => action.capitalize).try(:configuration_template)
Copy link
Contributor

@syncrou syncrou May 5, 2017

Choose a reason for hiding this comment

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

I believe this will fail without the to_s. There may be more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested and proved to_s can be dropped.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the latest changes I'm not able to get as far as before.

Using the following scenario:

  1. Create new Ansible Playbook catalog item with no retirement playbook.
  2. Edit the catalog item and add a new retirement playbook.
  3. Edit the catalog item and remove the retirement playbook.
  4. Delete the entire catalog item.

I am failing at 2. The retirement resource_action is created:

screen shot 2017-05-05 at 3 26 13 pm

But the retirement playbook is not created in Tower:

screen shot 2017-05-05 at 3 27 35 pm

@@ -225,6 +220,7 @@ def update_job_templates(name, description, config_info, auth_user)
ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScript.update_in_provider_queue(tower.id, params, auth_user)
else
delete_job_templates([job_template])
resource_actions.find_by(:action => action.capitalize).update_attributes(:configuration_template => nil)
Copy link
Contributor

@syncrou syncrou May 5, 2017

Choose a reason for hiding this comment

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

What do you think about passing the action to delete_job_templates? Then line 223 could be deleted.

  def delete_job_templates(job_templates, action = nil)
    auth_user = User.current_userid || 'system'
    job_templates.each do |job_template|
      ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScript
        .delete_in_provider_queue(job_template.manager.id, { :manager_ref => job_template.manager_ref }, auth_user)
    end
   resource_actions.find_by(:action => action.capitalize).update_attributes(:configuration_template => nil) if action
  end

@miq-bot
Copy link
Member

miq-bot commented May 5, 2017

Checked commits bzwei/manageiq@dcece3e~...2dc391f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

@syncrou
Copy link
Contributor

syncrou commented May 5, 2017

👍 Looks good to me

@gmcculloug gmcculloug merged commit fc36cd5 into ManageIQ:master May 8, 2017
@gmcculloug gmcculloug added this to the Sprint 60 Ending May 8, 2017 milestone May 8, 2017
simaishi pushed a commit that referenced this pull request May 8, 2017
Service Playbook updates fqname and configuration_template
(cherry picked from commit fc36cd5)

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

simaishi commented May 8, 2017

Fine backport details:

$ git log -1
commit 6abd7a6bcd96e2fad317aa05e6ec9932f39d8e62
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon May 8 08:58:09 2017 -0400

    Merge pull request #15007 from bzwei/retire_playbook_update
    
    Service Playbook updates fqname and configuration_template
    (cherry picked from commit fc36cd5817ec9ce58828870911b8880691d3a5e6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1448868

@bzwei bzwei deleted the retire_playbook_update branch May 8, 2017 13:45
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.

6 participants