Skip to content
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 new class Dialog::ContainerTemplateServiceDialog. #15216

Merged
merged 2 commits into from
Jun 7, 2017

Conversation

lfu
Copy link
Member

@lfu lfu commented May 24, 2017

This dialog is to be used by a container template service.

screen shot 2017-06-07 at 4 23 51 pm

https://www.pivotaltracker.com/story/show/145899183

@miq-bot assign @gmcculloug
@miq-bot add_label services, enhancement

cc @bzwei

@gmcculloug
Copy link
Member

@lfu Can you include a screenshot of a dialog created from these changes in the description.

end

def add_parameter_field(param, group, position)
group.dialog_fields.build(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have access to any parameter data at the moment, but I recall that there was enough information to include regex validation. Is that still possible?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some parameter data.

#<ContainerTemplateParameter:0x007f8086adabe8
  id: 18,
  name: "MQ_USERNAME",
  ems_created_on: nil,
  display_name: nil,
  description: "User name for standard broker user. It is required for connecting to the broker. If left empty, it will be generated.",
  value: nil,
  generate: "expression",
  from: "user[a-zA-Z0-9]{3}",
  required: nil,
  container_template_id: 2,
  created_at: Mon, 08 May 2017 18:46:17 UTC +00:00,
  updated_at: Mon, 08 May 2017 18:46:17 UTC +00:00>,
 #<ContainerTemplateParameter:0x007f8086adaa08
  id: 19,
  name: "MQ_PASSWORD",
  ems_created_on: nil,
  display_name: nil,
  description: "Password for standard broker user. It is required for connecting to the broker. If left empty, it will be generated.",
  value: nil,
  generate: "expression",
  from: "[a-zA-Z0-9]{8}",
  required: nil,
  container_template_id: 2,
  created_at: Mon, 08 May 2017 18:46:17 UTC +00:00,
  updated_at: Mon, 08 May 2017 18:46:17 UTC +00:00>

After understanding how they should be used, I will add support for them. Maybe in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex in from is used by Openshift to generate the value if empty. It is not used for validation. Take the above MQ_PASSWORD as an example which has from: "[a-zA-Z0-9]{8}". If the user leave the field empty, the value would be generated, like K0uVE3wN. If the user input the value like abc, or abcde12345 in the template, they would all be accepted as valid by Openshift.

@gmcculloug gmcculloug requested a review from bzwei May 24, 2017 19:33
@bzwei
Copy link
Contributor

bzwei commented May 25, 2017

@lfu You only covered the parameters part. We will need to find out what other options needed to do a container template provisioning.

:default_value => param.value,
:label => param.name.tr("_", " ").titleize,
:description => param.description,
:reconfigurable => true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to research whether container template provisioning can be reconfigured.

@lfu lfu force-pushed the container_template_dialog branch 6 times, most recently from ea2c512 to 41743da Compare June 2, 2017 16:30
Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bzwei This looks good to me and think we should merge these changes. Remaining work for the dialog will be exposing a project section to allow assigning to or creating a new project. That work should be in a separate PR.

@bzwei
Copy link
Contributor

bzwei commented Jun 7, 2017

Do we allow the user to select a project at ordering time? Then the dropdown for project should be in this dialog.

Should we move (generated if empty) to the tooltip?

@bzwei
Copy link
Contributor

bzwei commented Jun 7, 2017

@gmcculloug sorry, missed your comment already on the project issue. A separate PR is fine

@Loicavenel
Copy link

There are 2 options we are looking at.. deploying in an existing Project (list all existing project with RBAC), or deploying in a new project user will have entered in the dialog (Back-end is ready for it)

@lfu
Copy link
Member Author

lfu commented Jun 7, 2017

@Loicavenel Could it be the user have to add a new project first via UI task under Projects, then he can deploy to an existing project - the one just added?

@gmcculloug
Copy link
Member

@lfu We do this other places and it requires 2 fields. A text box and a dynamically filled drop-down showing existing projects. The text box takes precedence over the drop-down list.

@gmcculloug
Copy link
Member

@lfu Let's go with "(Auto-generated if empty)" and leave it in the label for now. Then we can get feedback and followup if we want to move this to hover-text.

@lfu lfu force-pushed the container_template_dialog branch from 41743da to 05c9598 Compare June 7, 2017 20:11
@miq-bot
Copy link
Member

miq-bot commented Jun 7, 2017

Checked commits lfu/manageiq@f7d15b2~...05c9598 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

@gmcculloug
Copy link
Member

@lfu Please update the dialog image in the PR description to match the recent changes.

@lfu
Copy link
Member Author

lfu commented Jun 7, 2017

@gmcculloug Updated.

@gmcculloug gmcculloug merged commit e73c175 into ManageIQ:master Jun 7, 2017
@gmcculloug gmcculloug added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 7, 2017
@lfu lfu deleted the container_template_dialog branch October 16, 2017 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants