-
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 Transform VM service dialog #13794
Conversation
ee1679c
to
d76e716
Compare
Depends on ManageIQ/manageiq-content#36 |
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.
Looks good to me, just a minor grammar suggestion.
ae_attributes: {} | ||
- name: provider | ||
description: Target infrastructure provider to which this virtual machine | ||
should be imported. |
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 be imported to.
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'm not a native speaker of course, but IMHO the 2 possibilities here are "... provider which this VM should be imported to" or "provider to which this VM should be imported", but not with 2 "to"s :-)
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.
Sure, two to are too much 👍 .
display_method_options: | ||
position: 0 | ||
dialog_groups: | ||
- description: Import given virtual machine to another infrastructure provider |
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.
@matobet
This might read better as
Import a virtual machine to another infrastructure provider
position: 0 | ||
dialog_fields: | ||
- name: name | ||
description: Name of the created virtual machine |
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.
@matobet
Is this an existing name or the name when it its moved to the new provider?
If its an existing name do we have to worry about users typing errors, when would we validate those?
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.
@mkanoor It is the name of the newly created VM in the destination provider. What might be helpful to the user is to pre-fill it with some default value (same as original, add _transformed
to the original name, ....)
@matobet Please add a screenshot of the dialog to the PR description. |
@gmcculloug done |
@matobet @gmcculloug @mkanoor the dialogs can now be seeded from provider plugins too: |
oh, sorry: #14668 this has to be merged for service dialogs... Care to wait? |
ok. sorry for the confusion. Let's merge here because of |
@miq-bot add_label fine/yes |
Checked commit matobet@4e0c100 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
Since the service dialogs seem to be sourced now exclusively from plugins (https://github.com/ManageIQ/manageiq/blob/master/app/models/dialog.rb#L28), migrated this dialog into the provider PR ManageIQ/manageiq-providers-ovirt@cb7bce4 One question I have is how this will be possible to backport to Fine, where the provider repo wasn't split yet - so there will be no plugin to put this dialog into.... @gmcculloug @durandom |
This will require a dedicated FINE PR then (this is always the case if the pathnames dont match 100%) |
@durandom Yes that will definitely the case but my question is on which repo will this PR be against? Since we cannot seed the dialogs from the core repo..... |
You'll seed your dialog in the ovirt repo and then open a [FINE] PR to backport it. |
@durandom yes, thank you, so I guess this one can be closed |
This PR adds the content of a
transform_vm
service dialog that consumes automation methods defined in ManageIQ/manageiq-content#36 to facilitate the transformation (import) of a infra VM between two providers.Screenshots
Links