-
Notifications
You must be signed in to change notification settings - Fork 897
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 to ServiceTemplateOrchestration #13628
add create_catalog_item to ServiceTemplateOrchestration #13628
Conversation
create(options.except(:config_info)).tap do |service_template| | ||
config_info = options[:config_info] | ||
|
||
service_template.orchestration_template = if config_info[:template_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.
Do we want to validate both template and manager must exist? If this is the only call we should unless users can call update method to set them later. This make me think of the need for update method and delete method as a complete solution.
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.
@bzwei yeah, good point. if both are required for a valid orchestration template, we should check that both exist, as well as make an update / delete method.
|
||
def self.validate_config_info(options) | ||
config_info = options[:config_info] | ||
if (config_info[:template_id] && !config_info[:manager_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.
You allow a case that neither exists. Let's make it simpler by requiring both must exist.
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.
@bzwei oh gotcha. was thinking it was either one or the other
create(options.except(:config_info)).tap do |service_template| | ||
config_info = validate_config_info(options) | ||
|
||
if config_info[:template_id] && config_info[:manager_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.
So here there is no need for the if
statement
:manager_id => manager.id, | ||
:provision => { | ||
:fqname => ra1.fqname, | ||
: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.
@jntullo Let's change :service_dialog_id
to :dialog_id
. This is inline with the column name for ResourceAction
You need to change in the base method too. Thanks.
create(options.except(:config_info)).tap do |service_template| | ||
config_info = validate_config_info(options) | ||
|
||
service_template.orchestration_template = OrchestrationTemplate.find(config_info[:template_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.
We want to accept :template_id
or :template
. The later points to an object that can be directly assigned. Same to manager. Need to update the validate method too.
|
||
def self.validate_config_info(options) | ||
config_info = options[:config_info] | ||
unless (config_info[:template_id] && config_info[:manager_id]) || (config_info[:template] && config_info[:manager]) |
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.
@bzwei should I check for combinations of ids / objects?
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.
Ideally we should allow the combination. What you have here is likely real. I am ok with it.
adding in a check on reload
Checked commits jntullo/manageiq@ce9b22b~...911d7fd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@miq-bot remove_label wip |
create(options.except(:config_info)).tap do |service_template| | ||
config_info = validate_config_info(options) | ||
|
||
service_template.orchestration_template = if config_info[:template_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.
We may simply put config_info[:template] || OrchestrationTemplate.find(config_info[:template_id])
since we already validate either one must exist.
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.
@bzwei I wanted to, but the line was too long so rubocop wasn't happy, and this was more readable than breaking that up onto two lines imo
@miq-bot assign @gmcculloug |
|
||
def self.validate_config_info(options) | ||
config_info = options[:config_info] | ||
unless (config_info[:template_id] && config_info[:manager_id]) || (config_info[:template] && config_info[:manager]) |
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.
@jntullo I would suggest allowing for the different combinations since that would better align with the logic in the create_catalog_item
method. This can be done in a followup PR.
This adds class method
create_catalog_item
to ServiceTemplateOrchestration and takes in the following:@miq-bot enhancement, euwe/no, services, wip
@miq-bot assign @bzwei