-
Notifications
You must be signed in to change notification settings - Fork 898
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
add create_catalog_item class method to ServiceTemplate #13589
add create_catalog_item class method to ServiceTemplate #13589
Conversation
@@ -45,6 +45,19 @@ class ServiceTemplate < ApplicationRecord | |||
virtual_has_one :custom_actions, :class_name => "Hash" | |||
virtual_has_one :custom_action_buttons, :class_name => "Array" | |||
|
|||
# This method creates | |||
def self.create_catalog_item(options, auth_user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to add get_catalog_item
in this PR?
create(options.except(:config_info)).tap do |service_template| | ||
config_info = options[:config_info].except(:ae_endpoints) | ||
workflow_class = MiqProvisionWorkflow.class_for_source(config_info[:src_vm_id]) | ||
raise 'Could not find Provision Workflow class for source VM' unless workflow_class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should allow to create a generic service template without workflow but possibly a subtype
@@ -45,6 +45,19 @@ class ServiceTemplate < ApplicationRecord | |||
virtual_has_one :custom_actions, :class_name => "Hash" | |||
virtual_has_one :custom_action_buttons, :class_name => "Array" | |||
|
|||
# This method creates | |||
def self.create_catalog_item(options, auth_user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add blank lines to improve the readability?
@@ -45,6 +45,19 @@ class ServiceTemplate < ApplicationRecord | |||
virtual_has_one :custom_actions, :class_name => "Hash" | |||
virtual_has_one :custom_action_buttons, :class_name => "Array" | |||
|
|||
# This method creates | |||
def self.create_catalog_item(options, auth_user) | |||
create(options.except(:config_info)).tap do |service_template| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume before the create
method you wanted to invalidate options
and add default values where applicable
@@ -45,6 +45,19 @@ class ServiceTemplate < ApplicationRecord | |||
virtual_has_one :custom_actions, :class_name => "Hash" | |||
virtual_has_one :custom_action_buttons, :class_name => "Array" | |||
|
|||
# This method creates | |||
def self.create_catalog_item(options, auth_user) | |||
create(options.except(:config_info)).tap do |service_template| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would include the whole create process in a transaction
block because multiple db records are created in this method and they should be rollback back if any error occurs.
:schedule_type => ['immediately', 'Immediately on Approval'], | ||
:instance_type => [flavor.id, flavor.name], | ||
:src_ems_id => [ems.id, ems.name], | ||
:provisioning => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change to :provision
expect(service_template.service_resources.count).to eq(1) | ||
expect(service_template.service_resources.first.resource_type).to eq('MiqRequest') | ||
expect(service_template.dialogs.first).to eq(service_dialog) | ||
expect(service_template.resource_actions.pluck(:action)).to include('Provision', 'Reconfigure', 'Retirement') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not always include the three resource actions. Only create as requested.
ae_endpoints ||= {} | ||
[ | ||
{:name => 'Provision', | ||
:param_key => :provisioning, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:provision
:method => 'default_retirement_entry_point', | ||
:args => []} | ||
].each do |action| | ||
ae_endpoint = ae_endpoints[action[:param_key]] || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip if ae_endpoint
is nil
If user wants to define an action without dialog and use default entry point, an empty hash should be used like
:config_info => {
:retirement => {}
}
options[:config_info].assert_valid_keys(CATALOG_ITEM_ATTRS) | ||
transaction do | ||
create(options.except(:config_info)).tap do |service_template| | ||
config_info = options[:config_info].except(:provisioning, :retirement, :reconfigure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:provision
@@ -45,6 +60,22 @@ class ServiceTemplate < ApplicationRecord | |||
virtual_has_one :custom_actions, :class_name => "Hash" | |||
virtual_has_one :custom_action_buttons, :class_name => "Array" | |||
|
|||
def self.create_catalog_item(options, auth_user) | |||
options[:config_info].assert_valid_keys(CATALOG_ITEM_ATTRS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced we want to validate keys this way which makes maintaining the list difficult. I suggest we not limit keys here and leave the backend further down to raise error if invalid keys are encountered.
I like your way to create a template create(options.except(:config_info))
. This way we can accept for example either options[:service_template_catalog_id] = 1
or options[:service_template_catalog]=catalog_obj
.
:description => 'a description', | ||
:config_info => { | ||
:miq_request_dialog_name => request_dialog.name, | ||
:service_dialog_id => service_dialog.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need of this line
@miq-bot assign @gmcculloug |
@jntullo can you update the final format in the description section of this PR? |
@bzwei done 👍 |
provisioning to provision removing validation update create_resource_actions
Checked commits jntullo/manageiq@ef6758a~...1564ad8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
LGTM @gmcculloug Can you take a final look? |
This is a class method on
ServiceTemplate
tocreate_catalog_item
.The format for the options passed in are as follows:
Much of this is based off of #12594. By creating these class methods, it will simplify the process of creating catalog items. Once these class methods are completed, #12594 will be updated to account for these changes with a much simpler API
@miq-bot assign @bzwei
@miq-bot add_label enhancement, euwe/no, services
cc: @h-kataria