-
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 ServiceTemplateContainerTemplate. #15356
Add ServiceTemplateContainerTemplate. #15356
Conversation
1664b9b
to
bfbfc77
Compare
bfbfc77
to
3e454c4
Compare
cc @bzwei |
# :config_info | ||
# :provision | ||
# :dialog_id (or) | ||
# :new_dialog_name |
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.
For this initial work we will not support new_dialog_name
. User needs to prepare a dialog in advance.
|
||
private | ||
|
||
def create_dialogs(config_info) |
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.
remove
end | ||
end | ||
|
||
def new_dialog_required?(info) |
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.
remove
info && info.key?(:new_dialog_name) && !info.key?(:dialog_id) | ||
end | ||
|
||
def create_new_dialog(dialog_name, container_template_parameters) |
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.
remove
|
||
transaction do | ||
create_from_options(options).tap do |service_template| | ||
dialog_ids = service_template.send(:create_dialogs, enhanced_config) |
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.
remove
# :dialog_id (or) | ||
# :new_dialog_name | ||
# :container_project_id | ||
# :new_project_name |
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.
if you are going to support new_project_name
, you need to have create_project
logic in the code which I cannot find.
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.
# :new_dialog_name | ||
# :container_project_id | ||
# :new_project_name | ||
# :container_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.
In your PR to provide available_container_projects
I saw you can get ext_management_system
from the template but I don't see you accept an ext_management_system
(or better name container_manager
) in the options. How do you set an ems in the catalog_item?
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 ManageIQ::Automate::Container::Openshift::Operations::AvailableProjects
got ext_management_system
from container_template
. It is not necessary to save it into the service template.
ef86268
to
652a842
Compare
class ServiceTemplateContainerTemplate < ServiceTemplateGeneric | ||
def self.default_provisioning_entry_point(_service_type) | ||
'/Service/Generic/StateMachines/GenericLifecycle/provision' | ||
end |
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.
@lfu You also need to override the default_retirement_entry_point
so you do not pick up the setting from the base class. See app/models/service_template.rb#L302-L304. You want to return nil
in this case.
config_info = validate_config_info(options[:config_info]) | ||
enhanced_config = config_info.deep_merge( | ||
:provision => { | ||
:configuration_template => ContainerTemplate.find(config_info.fetch_path(:provision, :container_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.
@lfu @bzwei I think we want to follow the pattern in ServiceTemplateOrchestration
and use template_id
here. https://github.com/ManageIQ/manageiq/blob/master/app/models/service_template_orchestration.rb#L11
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.
@gmcculloug When we developed Orchestration service there is no configuration_template in resource_action table, so the orchestration_template is added as a resource of the service template.
I think we now can use configuration_template field to store the container template, like the way we store job template.
b61f3c0
to
bf84e98
Compare
end | ||
|
||
def self.validate_config_info(info) | ||
info[:provision][:fqname] ||= default_provisioning_entry_point('atomic') if info.key?(:provision) |
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.
For Ansible PlayBook
services we are not exposing the provisioning entry point so this check would make sense there, but here the UI will allow for different automate entry points to be selected. cc @bzwei
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 misread this logic, the change is good as is. 👍
self.class.send(:validate_config_info, opts) | ||
end | ||
|
||
# override |
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.
Is this comment meaningful? Suggest dropping it.
# do nothing since no service resources for this template | ||
end | ||
|
||
# override |
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.
Same here.
3ab89b2
to
78f75d3
Compare
78f75d3
to
6507cfd
Compare
Checked commit lfu@6507cfd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
Add ServiceTemplateContainerTemplate for container template provisioning.
@miq-bot assign @gmcculloug
@miq-bot add_label enhancement, services, providers/containers