Destroy Ansible Playbook job templates #14461

syncrou · 2017-03-22T17:45:05Z

If an associated configuration_template exists then delete the associated JobTemplate

Otherwise simply delete the ServiceTemplateAnsiblePlaybook

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

syncrou · 2017-03-22T17:46:31Z

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

@miq-bot assign @bzwei

cc @gmcculloug, @h-kataria

gmcculloug · 2017-03-22T18:21:38Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script.rb

+
+      # Get the record in our database
+      # TODO: This needs to be targeted refresh so it doesn't take too long
+      EmsRefresh.queue_refresh(manager, nil, true)


Does this need to by synchronous? Can we just queue the refresh and not wait around for it?

yes, delete can be asynchronous. Even the update_in_provider refresh can be async.

I'll get the change in. Interestingly enough - since it isn't waiting for a task id the UI treats it like an asynchronous call - but I didn't realize the true argument was still hanging around.

bdunne

I'm not a fan of the line continuations, but the content looks good to me, so I won't hold up the PR for it. 👍

bdunne · 2017-03-22T18:49:56Z

spec/models/service_template_ansible_playbook_spec.rb

    end
  end
+
+  def prebuild_service_template


This could be:

def prebuild_service_template ret = {:provision => {:configuration_template => job_template}} expect(described_class).to receive(:create_job_templates).and_return(ret).at_least(:once) described_class.create_catalog_item(catalog_item_options_two, user).tap do |service_template| expect(service_template).to receive(:job_template).and_return(job_template).at_least(:once) end end

FTFY ;) return is a keyword

gmcculloug · 2017-03-22T21:44:18Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script.rb

      manager = ExtManagementSystem.find(manager_id)

+      name_map = {


Minor, but this should probably be a frozen constant.

bzwei · 2017-03-23T15:10:32Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script.rb

+      'create' => 'Creating',
+      'update' => 'Updating',
+      'delete' => 'Deleting'
+    }.freeze


Make it a private constant.

This was changed as requested.

bzwei · 2017-03-23T15:14:09Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script.rb

    end

    private

-    def create_or_update_in_provider_queue(action, manager_id, params, auth_user)
+    def create_update_or_delete_in_provider_queue(action, manager_id, params, auth_user)


maybe name it act_in_provider_queue or operate_in_provider_queue?

bzwei · 2017-03-23T15:16:51Z

app/models/service_template_ansible_playbook.rb

+      ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScript
+        .delete_in_provider_queue(playbook.manager.id, { :manager_ref => manager_ref }, auth_user)
+    end
+    super()


Depending on what happens to destroy - I wasn't able to simply call super if the current method accepted arguments, had to pass nothing via () - Unless you're aware of a better way to handle it.

There is pre-destroy hook. I don't know whether it is the better way to place the logic there. That's the reason I suggest we research the Rails way to clean up resources not through association.
@Fryguy @kbrock need your inputs.

Just calling super will forward the auth_user arg.
Calling super() will forward no args.
ActiveRecord::Base#destroy doesn't expect any args.

bzwei · 2017-03-23T15:19:29Z

app/models/service_template_ansible_playbook.rb

@@ -151,4 +151,17 @@ def update_catalog_item(options, auth_user = nil)
    end
    super
  end
+
+  def destroy(auth_user = 'system')


destroy and destroy! are AR methods. Should we override it/them this way? Can we add an argument?. Let's do some research on a Rails way to clean resources at destroying.

If we can set an accessor for auth_user - but that sounds like a custom pattern. Not sure how to allow for passing in any user - unless we store the user used to create the catalog_item in the options hash? - Definitely open to ideas.

bzwei · 2017-03-23T15:21:52Z

app/models/service_template_ansible_playbook.rb

+
+  def destroy(auth_user = 'system')
+    config_info = options[:config_info]
+    [:provision, :retirement, :reconfigure].each do |action|


This is not the right way to clean job templates. We should iterate resource_actions and get the job template from resource_action.configuration_template

@bzwei - Currently all CRUD instance methods are following this pattern. I would rather adjust the instance CRUD methods in one single way via resource_action.configuration_template via a followup PR.

For create and update, config_info is a passed-in option. Here for delete there is no config_info passed in. We don't need to follow a pattern. I would rather implement it correctly here because it is the PR all about.

bzwei · 2017-03-23T19:18:52Z

@syncrou I am thinking we may want to create delete_catalog_item method, in line with create_catalog_item and update_catalog_item methods that we already have.

In the base class ServiceTemplate the delete_catalog_item simply call its own destroy! method. In ServiceTemplateAnsiblePlaybook#delete_catalog_item we can

raise an error if there is at least one service still links to it;
remove job templates
call super#delete_catalog_item.

But this requires UI and API to call this method rather than the destroy method. I don't know whether it is appropriate.
cc @h-kataria @jntullo

gmcculloug · 2017-03-23T19:35:10Z

app/models/service_template_ansible_playbook.rb

@@ -151,4 +151,17 @@ def update_catalog_item(options, auth_user = nil)
    end
    super
  end
+
+  def destroy(auth_user = 'system')


@syncrou I would suggest that we determine the auth_user property this way instead of expecting the user to tell us:

def destroy auth_user = User.current_userid || 'system'

This keeps the method signature unchanged and gets the one thing we all want; using super instead of super(). 😉

jntullo · 2017-03-23T21:26:03Z

@bzwei a delete_catalog_item would be an easy fix to make to the API, and wouldn't affect the way it's used or called - not a breaking change.

bzwei · 2017-03-24T13:43:30Z

@jntullo @h-kataria We decided that we don't really need to introduce delete_catalog_item method.

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

miq-bot · 2017-03-24T14:52:29Z

Checked commit syncrou@731e8dd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

bdunne · 2017-03-24T22:22:07Z

Test failures are unrelated.

miq-bot added enhancement providers/ansible_tower lifecycle/services labels Mar 22, 2017

miq-bot assigned bzwei Mar 22, 2017

gmcculloug reviewed Mar 22, 2017

View reviewed changes

gmcculloug requested review from bdunne and bzwei March 22, 2017 18:21

syncrou force-pushed the delete_catalog_item_for_playbooks branch from ecdd380 to d2bc767 Compare March 22, 2017 18:38

bdunne reviewed Mar 22, 2017

View reviewed changes

syncrou force-pushed the delete_catalog_item_for_playbooks branch from d2bc767 to 35c9c0a Compare March 22, 2017 19:45

gmcculloug reviewed Mar 22, 2017

View reviewed changes

syncrou force-pushed the delete_catalog_item_for_playbooks branch from 35c9c0a to 88fa1ba Compare March 23, 2017 14:16

bzwei suggested changes Mar 23, 2017

View reviewed changes

syncrou force-pushed the delete_catalog_item_for_playbooks branch 2 times, most recently from 56237ec to 14f267f Compare March 23, 2017 17:12

gmcculloug reviewed Mar 23, 2017

View reviewed changes

syncrou force-pushed the delete_catalog_item_for_playbooks branch from 14f267f to d9f850d Compare March 23, 2017 19:55

syncrou force-pushed the delete_catalog_item_for_playbooks branch from d9f850d to 817b149 Compare March 24, 2017 14:30

Destroy Ansible Playbook job templates

731e8dd

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

syncrou force-pushed the delete_catalog_item_for_playbooks branch from 817b149 to 731e8dd Compare March 24, 2017 14:49

bzwei approved these changes Mar 24, 2017

View reviewed changes

gmcculloug approved these changes Mar 24, 2017

View reviewed changes

bdunne approved these changes Mar 24, 2017

View reviewed changes

gmcculloug merged commit a254593 into ManageIQ:master Mar 25, 2017

gmcculloug added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 25, 2017

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destroy Ansible Playbook job templates #14461

Destroy Ansible Playbook job templates #14461

syncrou commented Mar 22, 2017 •

edited

Loading

syncrou commented Mar 22, 2017

gmcculloug Mar 22, 2017

bzwei Mar 22, 2017

syncrou Mar 22, 2017

bdunne left a comment

bdunne Mar 22, 2017 •

edited by Fryguy

Loading

Fryguy Mar 22, 2017

gmcculloug Mar 22, 2017

bzwei Mar 23, 2017

syncrou Mar 23, 2017

bzwei Mar 23, 2017

bzwei Mar 23, 2017

syncrou Mar 23, 2017

bzwei Mar 23, 2017

bdunne Mar 23, 2017

bzwei Mar 23, 2017

syncrou Mar 23, 2017

bzwei Mar 23, 2017

syncrou Mar 23, 2017 •

edited

Loading

bzwei Mar 23, 2017

bzwei commented Mar 23, 2017

gmcculloug Mar 23, 2017 •

edited

Loading

jntullo commented Mar 23, 2017

bzwei commented Mar 24, 2017

miq-bot commented Mar 24, 2017

bdunne commented Mar 24, 2017

Destroy Ansible Playbook job templates #14461

Destroy Ansible Playbook job templates #14461

Conversation

syncrou commented Mar 22, 2017 • edited Loading

syncrou commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne left a comment

Choose a reason for hiding this comment

bdunne Mar 22, 2017 • edited by Fryguy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syncrou Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Mar 23, 2017

gmcculloug Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

jntullo commented Mar 23, 2017

bzwei commented Mar 24, 2017

miq-bot commented Mar 24, 2017

bdunne commented Mar 24, 2017

syncrou commented Mar 22, 2017 •

edited

Loading

bdunne Mar 22, 2017 •

edited by Fryguy

Loading

syncrou Mar 23, 2017 •

edited

Loading

gmcculloug Mar 23, 2017 •

edited

Loading