ServiceTemplate update_catalog_item #13811

jntullo · 2017-02-08T14:19:40Z

WIP PR for updating of a catalog item. Format for update will be the same as creating one, ie:

        {
          :name        => 'Updated Template Name',
          :prov_type   => 'amazon',
          :display     => 'false',
          :description => 'a description',
          :config_info => {
            :miq_request_dialog_name => request_dialog.name,
            :placement_auto          => [true, 1],
            :number_of_vms           => [1, '1'],
            :src_vm_id               => [new_vm.id, new_vm.name],
            :vm_name                 => new_vm.name,
            :schedule_type           => ['immediately', 'Immediately on Approval'],
            :instance_type           => [flavor.id, flavor.name],
            :src_ems_id              => [ems.id, ems.name],
            :provision               => {
              :fqname    => ra1.fqname,
              :dialog_id => nil
            },
            :reconfigure             => {
              :fqname    => ra3.fqname,
              :dialog_id => service_dialog.id
            }
          }
        }

@miq-bot add_label wip, enhancement, services

bzwei · 2017-02-08T22:16:32Z

app/models/service_template.rb

@@ -74,6 +74,25 @@ def self.create_catalog_item(options, auth_user)
    end
  end

+  def self.update_catalog_item(catalog_item, options, auth_user)


It should NOT be a class method.

miq-bot · 2017-02-09T14:46:40Z

This pull request is not mergeable. Please rebase and repush.

jntullo · 2017-02-10T17:12:48Z

@miq-bot remove_label wip

miq-bot · 2017-02-15T20:17:02Z

This pull request is not mergeable. Please rebase and repush.

syncrou · 2017-02-21T19:33:52Z

ping

bzwei · 2017-02-21T21:34:36Z

app/models/service_template.rb

+  def update_resource_actions(ae_endpoints)
+    resource_action_options.each do |action|
+      # If the action exists in updated parameters
+      if ae_endpoints[action[:param_key]]


You only need to find the existing resource_action once before the if/else clause.

bzwei · 2017-02-21T21:44:26Z

app/models/service_template.rb

-  def self.create_from_options(options)
-    create(options.except(:config_info).merge(:options => { :config_info => options[:config_info] }))
+  def update_from_options(options)
+    update_attributes!(options.except(:config_info).merge!(:options => { :config_info => options[:config_info] }))


should not need ! for merge thought it may not matter here.

bzwei · 2017-02-21T21:46:54Z

app/models/service_template_orchestration.rb

@@ -57,6 +57,29 @@ def self.validate_config_info(options)

  private

+  def update_service_resources(config_info, _auth_user = nil)
+    if config_info[:template_id] && config_info[:template_id] != orchestration_template.id


orchestration_template.try(:id), same for other similar cases.

bzwei · 2017-02-21T21:53:00Z

app/models/service_template_orchestration.rb

+
+  def validate_update_config_info(options)
+    super
+    config_info = options[:config_info]


code reuse - self.class.validate_config_info(options), same for the other class.

bzwei · 2017-02-21T21:56:06Z

spec/models/service_template_ansible_tower_spec.rb

-    it 'creates and returns an ansible tower catalog item' do
-      service_template = ServiceTemplateAnsibleTower.create_catalog_item(catalog_item_options)
-      service_template.reload
+    context '.create_catalog_item' do


Although context and describe seem interchangeable, my understanding the typical use is
describe a method
context a condition

bzwei · 2017-02-22T15:53:30Z

app/models/service_template.rb

+      if ae_endpoints[action[:param_key]]
+        # And the resource action exists on the template already, update it
+        if resource_action
+          resource_actions.find_by(:action => action[:name]).update_attributes!(ae_endpoints[action[:param_key]])


resource_action.update_attributes!

we can extract ae_endpoints[action[:param_key]] into a local variable since it is used three times

bzwei · 2017-02-22T16:01:05Z

app/models/service_template.rb

+    options[:config_info]
+  end
+
+  def resource_action_options


We can start to use the constants defined in ResourceAction for Provision, Reconfigure, and Retirement.

We use the word options every where. Can we come up a better name for this method? Something like resource_action_list. Naming is hard :(

@bzwei I think naming is my least favorite part of writing code 😆 I like resource_action_list - will do!

bzwei · 2017-02-22T16:08:13Z

app/models/service_template_ansible_tower.rb


  private

+  def update_service_resources(config_info, _auth_user = nil)
+    if config_info[:configuration_script_id] && config_info[:configuration_script_id] != job_template.id


job_template.try(:id)

bzwei · 2017-02-22T16:57:34Z

app/models/service_template.rb

+      if resource_params
+        # And the resource action exists on the template already, update it
+        if resource_action
+          resource_actions.find_by(:action => action[:name]).update_attributes!(resource_params)


resource_action.update_attributes!

bzwei · 2017-02-23T16:25:37Z

app/models/service_template.rb

+      if resource_params
+        # And the resource action exists on the template already, update it
+        if resource_action
+          resource_action.update_attributes!(resource_params)


We should slice selective attributes known to ResourceAction just like how to create resource actions.

miq-bot · 2017-02-23T18:21:59Z

This pull request is not mergeable. Please rebase and repush.

just remove the existing request and make a new one

syncrou · 2017-02-23T21:25:32Z

👍 Looks good to me

gmcculloug · 2017-02-24T14:29:49Z

app/models/service_template.rb

@@ -416,4 +473,8 @@ def resource_actions_info
    end
    config_info
  end
+
+  def resource_action_attrs


We need a better name here, this name is confusing/misleading. Also, I do not see why this is not just a constant instead of a method.

Maybe: RESOURCE_ACTION_UPDATE_ATTRS Open to other suggestions.

add .try(:destroy) make resource action attrs into a constant

miq-bot · 2017-02-24T14:52:26Z

Checked commits jntullo/manageiq@797e93d~...4a3301a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks good. 👍

miq-bot added enhancement lifecycle/services wip labels Feb 8, 2017

chessbyte assigned gmcculloug Feb 8, 2017

bzwei reviewed Feb 8, 2017

View reviewed changes

miq-bot added unmergeable and removed unmergeable labels Feb 9, 2017

jntullo changed the title ~~[WIP] ServiceTemplate update_catalog_item~~ ServiceTemplate update_catalog_item Feb 10, 2017

miq-bot removed the wip label Feb 10, 2017

miq-bot added the unmergeable label Feb 15, 2017

miq-bot removed the unmergeable label Feb 21, 2017

bzwei reviewed Feb 21, 2017

View reviewed changes

bzwei reviewed Feb 22, 2017

View reviewed changes

bzwei reviewed Feb 23, 2017

View reviewed changes

miq-bot added the unmergeable label Feb 23, 2017

Jillian Tullo added 5 commits February 23, 2017 13:36

update catalog item

797e93d

just remove the existing request and make a new one

remove request and add new one

f0755f2

add update_catalog_item to ansible tower and orchestration template

4aa2d8b

add update_service_resources

f8c2899

reuse code and cleanup specs

0b2704d

miq-bot removed the unmergeable label Feb 23, 2017

bzwei approved these changes Feb 23, 2017

View reviewed changes

gmcculloug reviewed Feb 24, 2017

View reviewed changes

slice resource_action_attrs

4a3301a

add .try(:destroy) make resource action attrs into a constant

gmcculloug merged commit 036a2cf into ManageIQ:master Feb 24, 2017

gmcculloug added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 24, 2017

jntullo mentioned this pull request Feb 24, 2017

Update service template API #14068

Merged

jntullo deleted the enhancement/update_service_template branch November 28, 2017 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ServiceTemplate update_catalog_item #13811

ServiceTemplate update_catalog_item #13811

jntullo commented Feb 8, 2017

bzwei Feb 8, 2017

miq-bot commented Feb 9, 2017

jntullo commented Feb 10, 2017

miq-bot commented Feb 15, 2017

syncrou commented Feb 21, 2017

bzwei Feb 21, 2017

bzwei Feb 21, 2017

bzwei Feb 21, 2017

bzwei Feb 21, 2017

bzwei Feb 21, 2017

bzwei Feb 22, 2017

bzwei Feb 22, 2017

bzwei Feb 22, 2017

bzwei Feb 22, 2017

jntullo Feb 22, 2017

bzwei Feb 22, 2017

bzwei Feb 22, 2017

bzwei Feb 23, 2017

miq-bot commented Feb 23, 2017

syncrou commented Feb 23, 2017

gmcculloug Feb 24, 2017

miq-bot commented Feb 24, 2017

ServiceTemplate update_catalog_item #13811

ServiceTemplate update_catalog_item #13811

Conversation

jntullo commented Feb 8, 2017

Choose a reason for hiding this comment

miq-bot commented Feb 9, 2017

jntullo commented Feb 10, 2017

miq-bot commented Feb 15, 2017

syncrou commented Feb 21, 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

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

miq-bot commented Feb 23, 2017

syncrou commented Feb 23, 2017

Choose a reason for hiding this comment

miq-bot commented Feb 24, 2017