-
Notifications
You must be signed in to change notification settings - Fork 120
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
v2v: Extend 'VM Transform' dialog to select VMs by tag #200
Conversation
Added tag category and tag name fields to the 'VM Transform' dialog to be able to select VMs for V2V by tag. Hide these fields if the dialog is open from VM details view. Hide VM name input field if the dialog is open from VM list view. Start several V2V requests if several VMs are selected by tag.
@miq-bot assign gmcculloug |
False positive from miq-bot: |
@smelamud This PR is missing specs. |
@@ -14,16 +14,36 @@ def initialize(handle = $evm) | |||
end | |||
|
|||
def main | |||
validate_root_args %w(vm dialog_name dialog_provider dialog_cluster dialog_storage dialog_sparse) | |||
validate_root_args(%w(vm dialog_provider dialog_cluster dialog_storage dialog_sparse)) | |||
if !@handle.root['dialog_tag_category'].empty? && !@handle.root['dialog_tag_name'].empty? |
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.
@smelamud Can we use @handle.root['dialog_tag_category'].present? && @handle.root['dialog_tag_name'].present?
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.
@tinaafitz Done.
validate_root_args(%w(vm dialog_provider dialog_cluster dialog_storage dialog_sparse)) | ||
if !@handle.root['dialog_tag_category'].empty? && !@handle.root['dialog_tag_name'].empty? | ||
tag = "/#{@handle.root['dialog_tag_category']}/#{@handle.root['dialog_tag_name']}" | ||
tagged_vms = @handle.vmdb(:vm).find_tagged_with(:all => tag, :ns => '/managed') |
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.
@smelamud Can we use the following to limit finding tagged VMWare VMs? $evm.vmdb('ManageIQ_Providers_Vmware_InfraManager_Vm').find_tagged_with(:all => tag, :ns => '/managed')
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.
@tinaafitz It is limited to VMWare VMs only now. In future it may support other providers.
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.
@smelamud I know the feature is limited to VMWare, but the vmdb find is not limited. Any Cloud/Infrastructure VM that contains that tag would be included here.
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.
@tinaafitz Done.
'Vm::vm' => @handle.root['vm'].id, | ||
'name' => @handle.root['dialog_name'], | ||
'Vm::vm' => vm.id, | ||
'name' => target_name.empty? ? vm.name : target_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.
Can you use present? in place of empty?
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.
@tinaafitz Done.
@@ -0,0 +1,43 @@ | |||
module ManageIQ |
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 a spec test for this method?
@@ -0,0 +1,47 @@ | |||
module ManageIQ |
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 a test for this method?
@@ -0,0 +1,32 @@ | |||
module ManageIQ |
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.
Test needed here as well.
@gmcculloug @tinaafitz Can I add specs in another pull request? I'm working on them currently, but cannot guarantee to get them ready by evening. |
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.
@smelamud Changes look good.
@gmcculloug Please review.
@gmcculloug @tinaafitz Added specs. |
Checked commits smelamud/manageiq-content@ee53f81~...51ceff7 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/methods/list_tag_names.rb
spec/content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/methods/list_tag_names_spec.rb
|
This warning are false. |
To clarify, the warnings are false because the |
Added tag category and tag name fields to the 'VM Transform' dialog to
be able to select VMs for V2V by tag. Hide these fields if the dialog is
open from VM details view. Hide VM name input field if the dialog is
open from VM list view. Start several V2V requests if several VMs are
selected by tag.
Depends on ManageIQ/manageiq-providers-ovirt#115
Depends on ManageIQ/manageiq-ui-classic#2501