create service template REST api #12594

jntullo · 2016-11-11T19:50:31Z

create service template REST api for use in the SUI - only doing atomic initially. Will follow up with composite.

@miq-bot add_label api, enhancement, wip, euwe/no
@miq-bot assign @abellotti

abellotti · 2016-11-16T15:10:23Z

config/api.yml

@@ -1386,6 +1388,8 @@
      :post:
      - :name: edit
        :identifier: catalogitem_edit
+      - :name: create
+        :identifier: catalogitem_new


why create action at the resource level ? probably only needed on collection.

jntullo · 2016-11-17T16:02:04Z

@bzwei @gmcculloug I worked on this based off of the atomic_req_submit here. Would appreciate some extra 👀 to ensure that I'm on the right track for creating atomic Service Templates.

Still need to add in the case for generic / composite.

bzwei · 2016-11-17T21:07:38Z

app/controllers/api/service_templates_controller.rb

@@ -4,5 +4,56 @@ class ServiceTemplatesController < BaseController
    include Subcollections::Tags
    include Subcollections::ResourceActions
    include Subcollections::ServiceRequests
+
+    def create_resource(_type, _id, data)


Shouldn't the method be named create_service_template unless resource is a term used by the API

it's a term used by the API

bzwei · 2016-11-17T21:11:46Z

app/controllers/api/service_templates_controller.rb

+
+    private
+
+    def create_atomic(data)


This name is also misleading. Looks like here you are creating type with a workflow. Generic types are also atomic.

generic can also be created through this method. will add in a test for that.

bzwei · 2016-11-17T21:15:04Z

app/controllers/api/service_templates_controller.rb

+
+    def validate_atomic_data(data)
+      raise 'Must provide request info' unless data.key?('request_info')
+      raise 'Provisioning Entry Point is required' unless data['request_info']['fqname']


Do we really want to enforce an provisioning entry point? I think now the backend have default value for all provisioning types.
In general I don't know whether it is necessary to add validation for too many fields, because even if some fields are missing at creation time, they can be added through update API.

Sounds good to me! Was just adding in the validations I saw in the other controller.

jntullo · 2016-11-18T18:05:35Z

@miq-bot remove_label wip

bzwei · 2016-11-18T19:11:42Z

app/controllers/api/service_templates_controller.rb

+          ra.update_attributes(:dialog => dialog, :fqname => request_data[action[:param_key]])
+        end
+      end
+      service_template


although no harm, what's the purpose of returning service_template?

bzwei · 2016-11-18T19:15:26Z

app/controllers/api/service_templates_controller.rb

+    def create_atomic(data)
+      service_template = ServiceTemplate.new(data.except('request_info'))
+      if data.key?('request_info')
+        service_template.add_resource(create_service_template_request(data['request_info']))


I think you need to make data['request_info'] with more structures instead of a plain hash. You code does not create service_template properly for generic_orchestration, generic_ansible_tower, and generic_container(to come). For these types different entries are added to service_resources table.

@bzwei exactly why I wanted this feedback 😃 is there a place in the code already doing this where I can follow along make it work for the REST api? Didn't quite see it in the existing controller...

bzwei · 2016-11-18T19:19:44Z

app/controllers/api/service_templates_controller.rb

+        {:name => 'Reconfigure', :param_key => 'reconfigure_fqname'},
+        {:name => 'Retirement', :param_key => 'retire_fqname'}
+      ].each do |action|
+        unless request_data[action[:param_key]].nil?


Even if the request_data does not have an action you'll still need to create an resource_action with default entry point. Look at ServiceTemplate and its subclasses for default entry points.

bzwei · 2016-11-21T17:17:17Z

app/controllers/api/service_templates_controller.rb

+      service_template = ServiceTemplate.new(data.except('request_info'))
+      dialog = nil
+      if data.key?('request_info')
+        service_template.add_resource(create_service_template_request(data['request_info']))


@jntullo Adding resources to a generic service_template, you can follow the examples in catalog_controller. Basically there is no common code. You need to call generic type specific methods to add them: https://github.com/ManageIQ/manageiq/blob/master/app/controllers/catalog_controller.rb#L1507 https://github.com/ManageIQ/manageiq/blob/master/app/controllers/catalog_controller.rb#L1509 https://github.com/ManageIQ/manageiq/blob/master/app/controllers/catalog_controller.rb#L1514

abellotti · 2016-11-21T21:45:23Z

app/controllers/api/service_templates_controller.rb

+      service_template.job_template = unless request_info['template_id'].nil?
+                                        ConfigurationScript.find(request_info['template_id'])
+                                      end
+    end


Quite a bit of code here. Any of these generic enough (base/foundation/verification) so they can live in the model at app/models/service_template.rb ?

Agreed @abellotti - once I had the logic down, was going to see about moving some of it into the model. 👍

bzwei · 2016-11-21T22:00:13Z

app/controllers/api/service_templates_controller.rb

+    end
+
+    def add_orchestration_template_vars(service_template, request_info)
+      return unless service_template.kind_of?(ServiceTemplateOrchestration)


it will never work because the type is always ServiceTemplate according to https://github.com/jntullo/manageiq/blob/4bf6307a7f67bb9a5223048fa122607ecc653e6a/app/controllers/api/service_templates_controller.rb#L19

Yes it does, it takes in a type parameter. It's also tested in the tests (checks that the count of ServiceTemplateOrchestration increases)

bzwei · 2016-11-21T22:00:25Z

app/controllers/api/service_templates_controller.rb

+    end
+
+    def add_ansible_tower_job_template_vars(service_template, request_info)
+      return unless service_template.kind_of?(ServiceTemplateAnsibleTower)


same as above

bzwei · 2016-11-21T22:52:51Z

app/controllers/api/service_templates_controller.rb

+      request
+    end
+
+    def set_provision_action(service_template, dialog, request_info)


why do you need to separate provision from retirement and reconfigure?

Mostly separated because the provisioning entrypoint takes in the service type, the others don't, and when I put them together rubocop yelled about complexity. Will try and refactor though.

bzwei · 2016-11-22T20:47:47Z

app/models/service_template.rb

+
+  def set_retire_reconfigure_actions(config_info, dialog)
+    [
+      {:name => 'Reconfigure', :param_key => 'reconfigure_fqname', :method => 'default_reconfiguration_entry_point'},


You can combine two methods by adding :args => [] to the hash, so it looks like

{:name => 'Provision', :param_key..., :method ..., :args => [service_type]}, {:name => 'Reconfigure', :param_key..., :method..., :args =>[]}, ...

Then self.class.send(action[:method], *action[:args])

bzwei · 2016-11-22T20:56:03Z

app/models/service_template.rb

@@ -258,6 +258,11 @@ def self.default_reconfiguration_entry_point
    nil
  end

+  def set_resource_actions(config_info, dialog)


I am not very convinced that we want this method in ServiceTemplate, especially here the keys in config_info are hard codes, obviously inherited from API controller. If we really want it here, can you document the use of this method?
@gmcculloug any thoughts?

Thoughts @gmcculloug?

Thanks for the reminder. Discussed with @bzwei before the break and I am not sure the best place for this code at the moment.

@abellotti Any suggestions on where this method should live.

jntullo · 2017-01-03T14:52:07Z

ping @gmcculloug

abellotti · 2017-01-04T22:28:49Z

app/models/service_template.rb

+    end
+    save!
+  end
+


Hi @jntullo, I've looked at this with @gmcculloug a bit,
this method can stay here in app/models/service_template.rb, seems generic enough, the only thing that took a while to understand was the config_info.

Can you change the signature in this method (and update the calling sequence in API accordingly) as follows:

def set_resource_actions(ae_endpoints, dialog)

where ae_endpoints would be the following hash:

{ :provisioning => "/.....", :retirement => "/.....", :reconfigure => "/....." }

non-existent/nil entries honored as you have it today.

Thanks,

abellotti · 2017-01-05T21:22:25Z

Thanks @jntullo for the update. LGTM!!

@gmcculloug @bzwei ok with you ?

abellotti · 2017-01-12T17:02:34Z

ping @gmcculloug @bzwei

bzwei · 2017-01-12T19:48:32Z

app/controllers/api/service_templates_controller.rb

+      service_template = ServiceTemplate.new(data.except('config_info'))
+      config_info = data['config_info'].except('ae_endpoints') || {}
+      case service_template.type
+      when 'ServiceTemplateOrchestration'


We are going to add new type ServiceTemplateContainer, and possibly other types in the future. How do we remember to come back to update this case...when?
The API controller attempts to create a service template, but the actual ServiceTemplate has many subclasses. Need a generic way to handle subtypes.

@bzwei good question. Almost sounds like it should be a part of the initialization process

@jntullo I am currently developing logic to create service template for the new AnsiblePlaybook service type. I think for each type we should have a class method to create a full stack of catalog item definition, meaning ServiceTemplate, ServiceResource, and ResourceActions. We can work together to move us to the same direction.

that sounds good to me @bzwei

@bzwei are you okay with this PR as is, and then enhancing it as the other work gets completed?

@jntullo We can merge this one if it becomes a blocker for other development work; otherwise I would think this is the right place to refactor.

I started to add the following to service_template.rb

# create ServiceTemplate and supporting ServiceResources and ResourceActions def self.create_catalog_item(options) end

The implementation is empty right now. You can move logic to this block and corresponding subclasses.
The method should return an instance of ServiceTemplate or subclass.

The method name can be debatable; I could not think of a better name.

any update on this PR ?

@abellotti I will be updating this soon (this week) based off of the class methods @bzwei and I have been working on

bzwei · 2017-02-02T15:48:39Z

app/controllers/api/service_templates_controller.rb

+
+    def create_resource(_type, _id, data)
+      raise 'Must provide a type' unless data['type']
+      type = data['type'].constantize


Here we have both _type and type. Consider to rename either one to something more meaningful.

bzwei · 2017-02-02T15:52:55Z

spec/requests/api/service_templates_spec.rb

+          :instance_type           => [flavor.id, flavor.name],
+          :src_ems_id              => [ems.id, ems.name],
+          :provision               => {
+            :fqname => ra1.fqname,


In reality you still need a service dialog for provisioning.

bzwei · 2017-02-02T15:55:07Z

LGTM except for the two minor comments.

imtayadeway · 2017-02-09T18:50:58Z

Please rebase this PR to incorporate #13846 in order to ensure api.yml integrity ❤️ ❤️ ❤️

jntullo · 2017-02-13T18:57:31Z

ping @abellotti

abellotti · 2017-02-13T20:13:30Z

app/controllers/api/service_templates_controller.rb

@@ -4,5 +4,12 @@ class ServiceTemplatesController < BaseController
    include Subcollections::Tags
    include Subcollections::ResourceActions
    include Subcollections::ServiceRequests
+
+    def create_resource(type, _id, data)
+      catalog_item_type = data['type'].try(:constantize) || collection_class(type)


we really shouldn't be running constantize on user specified data. Prefer if model returned the class for us. Are these known classes, or subclasses of known base classes ?

see app/models/miq_request's class_from_request_data() that we call from API. Might need something similar here.

jntullo · 2017-02-13T21:50:49Z

app/models/service_template.rb

+    "openstack"                => :ServiceTemplate,
+    "redhat"                   => :ServiceTemplate,
+    "vmware"                   => :ServiceTemplate
+  }.freeze


@bzwei Can you verify that these are accurate?

wouldn't logic like the following work here ?

return klass if request_type == ServiceTemplate.name request_klass = ServiceTemplate.descendants.detect { |sub_klass| request_type == sub_klass.name } return request_klass if request_klass

since:

ServiceTemplate.descendants.collect(&:name) => ["ServiceTemplateAnsibleTower", "ServiceTemplateGeneric", "ServiceTemplateOrchestration", "ServiceTemplateAnsiblePlaybook"]

miq-bot · 2017-02-13T21:51:28Z

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

bzwei · 2017-02-13T21:56:15Z

app/models/service_template.rb

@@ -75,6 +88,12 @@ def self.create_catalog_item(options, auth_user)
    end
  end

+  def self.class_from_request_data(data)
+    request_type = data['prov_type']
+    model_symbol = PROV_TYPE_TO_MODEL[request_type] || raise(ArgumentError, 'Invalid prov_type')


We can/should avoid lookup from a hash and prefer naming convention. If the type starts with generic_, we can do an auto conversion from generic_abc to ServiceTemplateAbc; for all the rest, directly ServiceTemplate.

sounds good, thanks @bzwei

miq-bot · 2017-02-13T22:22:01Z

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

remove resource action always set entry point

moving some methods to the model

default to ServiceTemplate

miq-bot · 2017-02-14T14:17:21Z

Checked commits jntullo/manageiq@ec70f64~...98d8237 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 🏆

abellotti · 2017-02-15T18:26:05Z

app/models/service_template.rb

+    request_type = data['prov_type']
+    if request_type.include?('generic_')
+      generic_type = request_type.split('generic_').last
+      "ServiceTemplate#{generic_type.camelize}".constantize


/cc @bzwei @gmcculloug you guys ok with this constantize ?

abellotti · 2017-02-15T20:15:44Z

Thanks @bzwei and @jntullo for this API Enhancement. LGTM!!

bzwei · 2017-02-15T20:36:10Z

@jntullo Good job!

miq-bot added api enhancement euwe/no wip labels Nov 11, 2016

miq-bot assigned abellotti Nov 11, 2016

abellotti reviewed Nov 16, 2016

View reviewed changes

bzwei reviewed Nov 17, 2016

View reviewed changes

jntullo changed the title ~~[WIP] create service template REST api~~ create service template REST api Nov 18, 2016

miq-bot removed the wip label Nov 18, 2016

bzwei suggested changes Nov 18, 2016

View reviewed changes

bzwei reviewed Nov 21, 2016

View reviewed changes

abellotti reviewed Nov 21, 2016

View reviewed changes

bzwei reviewed Nov 21, 2016

View reviewed changes

bzwei reviewed Nov 22, 2016

View reviewed changes

abellotti reviewed Jan 4, 2017

View reviewed changes

bzwei reviewed Jan 12, 2017

View reviewed changes

jntullo mentioned this pull request Jan 19, 2017

add create_catalog_item class method to ServiceTemplate #13589

Merged

bzwei reviewed Feb 2, 2017

View reviewed changes

bzwei approved these changes Feb 2, 2017

View reviewed changes

miq-bot added wip and removed wip labels Feb 6, 2017

abellotti reviewed Feb 13, 2017

View reviewed changes

jntullo commented Feb 13, 2017

View reviewed changes

miq-bot added the unmergeable label Feb 13, 2017

bzwei reviewed Feb 13, 2017

View reviewed changes

miq-bot removed the unmergeable label Feb 13, 2017

miq-bot added the unmergeable label Feb 13, 2017

Jillian Tullo added 8 commits February 14, 2017 08:13

add create service template to REST api

ec70f64

ability to create atomic service templates

3882e29

additional validations and specs

9f409ba

removing validations

25e3d83

remove resource action always set entry point

add support for generic service template orchestration

916c5ec

moving some methods to the model

updating config_info to include ae_endpoints

0d10632

use create_catalog_item

8e807e1

default to ServiceTemplate

update product feature

e90405f

miq-bot removed the unmergeable label Feb 14, 2017

adding class_from_request_data to ServiceTempalte

98d8237

abellotti reviewed Feb 15, 2017

View reviewed changes

abellotti merged commit 8e256a6 into ManageIQ:master Feb 15, 2017

abellotti added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 15, 2017

chrisarcand deleted the create_catalog_item branch November 28, 2017 19:26

create service template REST api #12594

create service template REST api #12594

Conversation

jntullo commented Nov 11, 2016 • edited by miq-bot Loading

Choose a reason for hiding this comment

jntullo commented Nov 17, 2016

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

jntullo commented Nov 18, 2016

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 Nov 21, 2016 • 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

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

jntullo commented Jan 3, 2017

Choose a reason for hiding this comment

abellotti commented Jan 5, 2017 • edited Loading

abellotti commented Jan 12, 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

jntullo Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Feb 2, 2017

imtayadeway commented Feb 9, 2017

jntullo commented Feb 13, 2017

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 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Feb 13, 2017

miq-bot commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Feb 15, 2017

bzwei commented Feb 15, 2017

jntullo commented Nov 11, 2016 •

edited by miq-bot

Loading

bzwei Nov 21, 2016 •

edited

Loading

abellotti commented Jan 5, 2017 •

edited

Loading

jntullo Jan 24, 2017 •

edited

Loading