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

Create or delete a catalog item on update #14830

Conversation

@syncrou
Copy link
Contributor

syncrou commented Apr 20, 2017

When updating an existing AnsiblePlaybook catalog item - add the ability to add a new retirement playbook if one didn't previously exist before: Case A

Also, allow deleting of an existing job_template if the update does not pass in a :playbook_id:
Case B

Further more: If you create a new catalog item, delete it, and then create a new one with the same name as the old one you will no longer receive an error: Case C

Testing notes:

Case A

  1. Create new Ansible Playbook Catalog item without a retirement playbook
  2. Edit the above Catalog Item and add a retirement playbook.

Case B

  1. Create new Ansible Playbook Catalog item without a retirement playbook
  2. Edit the above Catalog Item and add a retirement playbook.
  3. Edit the above Catalog Item again and delete the retirement playbook.

Case C

  1. Create a new catalog item
  2. Delete that catalog item
  3. Create a new catalog item with the same name as in part 1.

https://www.pivotaltracker.com/story/show/141868401

https://www.pivotaltracker.com/story/show/141265431

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

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

syncrou added 3 commits Apr 13, 2017
When updating an existing AnsiblePlaybook catalog item - add the ability to add a new retirement playbook if one didn't previously exist before

Testing notes:

Create new Ansible Playbook Catalog item without a retirement playbook
Edit the above Catalog Item and add a retirement playbook.

https://www.pivotaltracker.com/story/show/141868401

https://bugzilla.redhat.com/show_bug.cgi?id=1438839
When updating an existing AnsiblePlaybook catalog item - add the ability to add a new retirement playbook if one didn't previously exist before

Also, allow deleting of an existing job_template if the update does not pass in a playbok id

https://www.pivotaltracker.com/story/show/141868401

https://www.pivotaltracker.com/story/show/141265431

https://bugzilla.redhat.com/show_bug.cgi?id=1438839
@syncrou

This comment has been minimized.

Copy link
Contributor Author

syncrou commented Apr 20, 2017

@miq-bot assign @bzwei

@miq-bot add_label bug, providers/ansible_tower, services

cc - @gmcculloug

@syncrou syncrou force-pushed the syncrou:deux_1438839_create_or_delete_catalog_item_on_update branch to 693af1a Apr 20, 2017
end
end
end

def update_catalog_item(options, auth_user = nil)
config_info = validate_update_config_info(options)
name = options[:name]

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor

name = options[:name] || name
same for description


new_dialog = create_new_dialog(info[:new_dialog_name], job_template(action), info[:hosts]) if info[:new_dialog_name]
config_info[action][:dialog_id] = new_dialog.id if new_dialog
new_dialogs(config_info)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor
  1. Method names: I prefer to use verbs for method names, except nouns for getters.
  2. Adjust the order: update_job_templates, create_job_templates, create_dialogs
  3. Let's reuse the class method create_job_templates, like self.class.create_job_templates(name, description, config_info, auth_user, self)
def create_job_template(service_name, description, config_info, auth_user)
[:provision, :retirement, :reconfigure].each_with_object({}) do |action, hash|
next unless new_job_template_required(config_info[action], action)
hash[action] = { :configuration_template => self.class.send(:create_job_template, "miq_#{service_name}_#{action}", description, config_info[action], auth_user),

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor

extract "miq_#{service_name}_#{action}" into a method

info && job_template(action).nil? && info.key?(:playbook_id)
end

def create_job_template(service_name, description, config_info, auth_user)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor

no need

@@ -71,12 +71,17 @@ def create_new_dialog(dialog_name, job_template, hosts)

def self.create_job_templates(service_name, description, config_info, auth_user)
[:provision, :retirement, :reconfigure].each_with_object({}) do |action, hash|
next unless config_info[action] && config_info[action].key?(:playbook_id)
next unless new_job_template_required(config_info[action])

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor
next unless new_job_template_required?(config_info[action], action, service_template)
def job_template_modifications(name, description, config_info, auth_user)
[:provision, :retirement, :reconfigure].each do |action|
info = config_info[action]
if info && info.key?(:playbook_id) && !info.key?(:create_only)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor
if job_template(action)

no need to add :create_only, but this method needs to be called before creating new job templates.

hash[action] = { :configuration_template => create_job_template("miq_#{service_name}_#{action}", description, config_info[action], auth_user) }
end
end
private_class_method :create_job_templates

def self.new_job_template_required(info)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor
def self.new_job_template_required?(info, action, service_template)
  info && info.key?(:playbook_id) && service_template.job_template(action).nil?
end
@@ -71,12 +71,17 @@ def create_new_dialog(dialog_name, job_template, hosts)

def self.create_job_templates(service_name, description, config_info, auth_user)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor
def self.create_job_templates(service_name, description, config_info, auth_user, service_template = nil)
@@ -190,4 +217,20 @@ def delete_job_templates(job_templates)
.delete_in_provider_queue(job_template.manager.id, { :manager_ref => job_template.manager_ref }, auth_user)
end
end

def new_dialog_required(info)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor

add ? to the method name

info && info.key?(:new_dialog_name)
end

def new_job_template_required(info, action)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 21, 2017

Contributor

no need.

@syncrou

This comment has been minimized.

Copy link
Contributor Author

syncrou commented Apr 24, 2017

@bzwei - I believe I have addressed your feedback.

cc - @gmcculloug

app/models/service_template_ansible_playbook.rb Outdated
name = options[:name]
description = options[:description]
def job_template(action)
resource = resource_actions.find_by(:action => action.to_s.capitalize)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 25, 2017

Contributor

can we do

resource_actions.find_by(:action => action.to_s.capitalize).try(:configuration_template)

?

app/models/service_template_ansible_playbook.rb Outdated
def update_job_templates(name, description, config_info, auth_user)
[:provision, :retirement, :reconfigure].each do |action|
info = config_info[action]
next unless info

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 25, 2017

Contributor

Since job_template may not exist, we should never call job_template!. We do not need to update a job template if it does not exist. Therefore the logic should be like this

  next unless info   # no intention to change anything for this action
  
  job_template = job_template(action)
  next unless job_template   # job_template does not exist
  
  if info.key?(:playbook_id) # job_template exists, and playbook is given, try an update
    # update
  else                       # job_template exists but no more playbook, try a delete
    # delete
  end
app/models/service_template_ansible_playbook.rb Outdated
@@ -134,26 +144,55 @@ def validate_update_config_info(options)
self.class.send(:validate_config_info, opts)
end

def job_template(action)
def job_template!(action)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 25, 2017

Contributor

Can you double check existing code where job_template was called? Do they need to change to job_template!?

app/models/service_template_ansible_playbook.rb Outdated
end
end
private_class_method :create_job_templates

def self.new_job_template_required(info, action, service_template)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 25, 2017

Contributor

self.new_job_template_required?

Can you move this method down next to instance method new_dialog_required?

@syncrou syncrou force-pushed the syncrou:deux_1438839_create_or_delete_catalog_item_on_update branch 5 times, most recently Apr 25, 2017
app/models/service_template_ansible_playbook.rb Outdated
end
end
private_class_method :create_job_templates

def self.new_job_template_required(info, action, service_template)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 25, 2017

Contributor

this method is no longer needed.

app/models/service_template_ansible_playbook.rb Outdated

new_dialog = create_new_dialog(info[:new_dialog_name], job_template(action), info[:hosts]) if info[:new_dialog_name]
config_info[action][:dialog_id] = new_dialog.id if new_dialog
def new_job_templates(config_info, name, description, auth_user)

This comment has been minimized.

Copy link
@bzwei

bzwei Apr 25, 2017

Contributor

new_job_templates is no longer needed

syncrou added 2 commits Apr 24, 2017
Allow the around destroy callback to also prepend to the front of the callback chain
@syncrou syncrou force-pushed the syncrou:deux_1438839_create_or_delete_catalog_item_on_update branch to f44a845 Apr 25, 2017
@bzwei
bzwei approved these changes Apr 25, 2017
Change all methods not specifically needed by the UI into private methods
Alphabetize all methods under private
@syncrou syncrou force-pushed the syncrou:deux_1438839_create_or_delete_catalog_item_on_update branch to f7e80bf Apr 25, 2017
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Apr 25, 2017

Checked commits syncrou/manageiq@4bcb4bd~...f7e80bf with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. ⭐️

@gmcculloug gmcculloug added the fine/yes label Apr 25, 2017
@syncrou

This comment has been minimized.

Copy link
Contributor Author

syncrou commented Apr 25, 2017

@miq-bot add_label blocker

@miq-bot miq-bot added the blocker label Apr 25, 2017
@gmcculloug gmcculloug merged commit 258dd18 into ManageIQ:master Apr 25, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 43.832%
Details
simaishi added a commit that referenced this pull request Apr 26, 2017
…catalog_item_on_update

Create or delete a catalog item on update
(cherry picked from commit 258dd18)

https://bugzilla.redhat.com/show_bug.cgi?id=1445894
https://bugzilla.redhat.com/show_bug.cgi?id=1445942
@simaishi

This comment has been minimized.

Copy link
Contributor

simaishi commented Apr 26, 2017

Fine backport details:

$ git log -1
commit b82c87a1f49f94afcc94570eb8db5f11c651b924
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Apr 25 17:30:30 2017 -0400

    Merge pull request #14830 from syncrou/deux_1438839_create_or_delete_catalog_item_on_update
    
    Create or delete a catalog item on update
    (cherry picked from commit 258dd189206418143b9f948bbc93aefac972ee66)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1445894
    https://bugzilla.redhat.com/show_bug.cgi?id=1445942
@simaishi simaishi added fine/backported and removed fine/yes labels Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.