Modified destroying an Ansible Service Template #14586

syncrou · 2017-03-30T20:02:30Z

We no longer will destroy a service template if there is a retirement configuration
that has not been retired

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

syncrou · 2017-03-30T20:03:17Z

@miq-bot add_label services, enhancement

@miq-bot assign @gmcculloug

-cc @bzwei

bzwei · 2017-03-30T20:11:05Z

app/models/service_template_ansible_playbook.rb

@@ -156,11 +156,17 @@ def update_catalog_item(options, auth_user = nil)

  def destroy
    auth_user = User.current_userid || 'system'
+    raise 'Attached retirement configurations do not allow the service template to be deleted.' if retirement_defined?


better use before_destroy statement than raise an error here.

@bzwei - In the case where I would need to return an error - do we append the message to an errors hash? - Not sure the pattern we use in our app for that callback.

You can look at OrchestrationTemplate as an example.

Suggest changing the text to something like:

raise 'Destroy aborted. Active Services require retirement resources associated with this instance.' if ...

bzwei · 2017-03-30T20:28:47Z

app/models/service_template_ansible_playbook.rb

    resource_actions.where.not(:configuration_template_id => nil).each do |resource_action|
      job_template = resource_action.configuration_template
      ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScript
        .delete_in_provider_queue(job_template.manager.id, { :manager_ref => job_template.manager_ref }, auth_user)
    end
    super
  end
+
+  def retirement_defined?
+    retirement_template = resource_actions.where("action = 'Retirement' AND configuration_template_id IS NOT NULL").present?


resource_actions.where(:action => 'Retirement').where.not(:configuration_template => nil).exists?

variable retirement_template is then misleading; suggest something like retirement_jt_exist

bzwei · 2017-03-30T20:35:50Z

app/models/service_template_ansible_playbook.rb

    resource_actions.where.not(:configuration_template_id => nil).each do |resource_action|
      job_template = resource_action.configuration_template
      ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScript
        .delete_in_provider_queue(job_template.manager.id, { :manager_ref => job_template.manager_ref }, auth_user)
    end
    super
  end
+
+  def retirement_defined?


Need comments on the purpose of this method.
This public method may be called by UI. Consider a better name.

@bzwei - I was debating on making it private - but thought it may have public use. Should it just be made private?

Need a public method and a private one. The private one is used by before_destroy which may throw an error.

@bzwei - looks like I missed this request - I'll get this change in.

The new method names do not seem accurate to me. retirement_defined? does not indicate that we are looking for active services and undefined_retirement? makes me thing the retirement methods are un-defining the retirement methods or something.

Maybe:
retirement_resources_in_use?
check_retirement_on_destroy

gmcculloug · 2017-03-31T16:07:39Z

app/models/service_template_ansible_playbook.rb

+
+  private
+
+  def check_for_retirement_potential


Two minor suggestions:

The word "for" is not needed. check_retirement_potential.

Rename the other method `retirement_potential?' instead of swapping the order.

bzwei · 2017-03-31T16:08:33Z

app/models/service_template_ansible_playbook.rb

+  #   2. At least one service instance where :retired is set to false.
+  #
+  def potential_retirement?
+    retirement_jt_exists = resource_actions.where("action = 'Retirement' AND configuration_template_id IS NOT NULL").present?


@syncrou you missed the previous comment. The better way can be resource_actions.where(:action => 'Retirement').where.not(:configuration_template => nil).exists?

We no longer will destroy a service template if there is a retirement configuration that has not been retired https://www.pivotaltracker.com/story/show/142646939

miq-bot · 2017-03-31T16:57:45Z

Checked commit syncrou@492c00b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 👍

bzwei · 2017-03-31T17:39:33Z

@gmcculloug should this PR have fine/yes?

Modified destroying an Ansible Service Template (cherry picked from commit 4ea0b83)

simaishi · 2017-03-31T21:29:10Z

Fine backport details:

$ git log -1
commit 5a491f06d3fe3eedc67a34e1c550cd81b30b6501
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Fri Mar 31 13:18:23 2017 -0400

    Merge pull request #14586 from syncrou/raise_error_on_delete
    
    Modified destroying an Ansible Service Template
    (cherry picked from commit 4ea0b83e10934f8cf8f0ca0ad86b03eebd1d9b7a)

miq-bot added enhancement lifecycle/services labels Mar 30, 2017

miq-bot assigned gmcculloug Mar 30, 2017

bzwei reviewed Mar 30, 2017

View reviewed changes

syncrou force-pushed the raise_error_on_delete branch 2 times, most recently from 1391180 to ae8b200 Compare March 31, 2017 15:38

gmcculloug reviewed Mar 31, 2017

View reviewed changes

bzwei reviewed Mar 31, 2017

View reviewed changes

Modified destroying an Ansible Service Template

492c00b

We no longer will destroy a service template if there is a retirement configuration that has not been retired https://www.pivotaltracker.com/story/show/142646939

syncrou force-pushed the raise_error_on_delete branch from ae8b200 to 492c00b Compare March 31, 2017 16:54

gmcculloug merged commit 4ea0b83 into ManageIQ:master Mar 31, 2017

gmcculloug added this to the Sprint 58 Ending Apr 10, 2017 milestone Mar 31, 2017

syncrou deleted the raise_error_on_delete branch March 31, 2017 17:51

gmcculloug added the fine/yes label Mar 31, 2017

simaishi pushed a commit that referenced this pull request Mar 31, 2017

Merge pull request #14586 from syncrou/raise_error_on_delete

5a491f0

Modified destroying an Ansible Service Template (cherry picked from commit 4ea0b83)

simaishi added fine/backported and removed fine/yes labels Mar 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified destroying an Ansible Service Template #14586

Modified destroying an Ansible Service Template #14586

syncrou commented Mar 30, 2017

syncrou commented Mar 30, 2017

bzwei Mar 30, 2017

syncrou Mar 30, 2017

bzwei Mar 30, 2017

gmcculloug Mar 30, 2017

bzwei Mar 30, 2017

bzwei Mar 30, 2017 •

edited

Loading

syncrou Mar 30, 2017 •

edited

Loading

bzwei Mar 30, 2017

syncrou Mar 31, 2017

gmcculloug Mar 31, 2017

gmcculloug Mar 31, 2017

bzwei Mar 31, 2017

miq-bot commented Mar 31, 2017

bzwei commented Mar 31, 2017

simaishi commented Mar 31, 2017

Modified destroying an Ansible Service Template #14586

Modified destroying an Ansible Service Template #14586

Conversation

syncrou commented Mar 30, 2017

syncrou commented Mar 30, 2017

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

bzwei Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

syncrou Mar 30, 2017 • edited 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

miq-bot commented Mar 31, 2017

bzwei commented Mar 31, 2017

simaishi commented Mar 31, 2017

bzwei Mar 30, 2017 •

edited

Loading

syncrou Mar 30, 2017 •

edited

Loading