-
Notifications
You must be signed in to change notification settings - Fork 357
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
Reconfigure VM: Add / Remove Network Adapters #3121
Conversation
|
This pull request is not mergeable. Please rebase and repush. |
Please disregard the remaining 3 codeclimate issues: Cognitive Complexity of 6 (exceeds 5) and two similar blocks of code not suitable for refactoring. |
@miq-bot remove-label pending core Tested the UI changes while testing ManageIQ/manageiq-providers-vmware#163 👍 |
@@ -61,6 +65,20 @@ ManageIQ.angular.app.controller('reconfigureFormController', ['$http', '$scope', | |||
vm.reconfigureModel.enableAddDiskButton = (nrDisksAfterReconfigure < 4); | |||
}; | |||
|
|||
vm.setEnableAddNetworkAdapterButton = function() { | |||
var nrNetworksAfterReconfigure = 0; | |||
angular.forEach(vm.reconfigureModel.vmNetworkAdapters, function(network) { |
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.
var nrNetworksAfterReconfigure = _.reject(vm.reconfigureModel.vmNetworkAdapters, { add_remove: 'remove' }).length;
?
(Unless the switch is meaningful 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.
@himdel Good refactoring suggestion. Thanks.
|
||
# determine available switches for this host... | ||
switch_ids = [] | ||
HostSwitch.where("host_id = ?", host_id).each do |host_switch| |
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.
Missing RBAC? (And with Lan.where)
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.
@himdel Could you elaborate on that?
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.
@himdel Do you mean add filter_by_tags as in miq_provision_virt_workflow.rb ?
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, sorry.. I think you need Rbac.filtered( HostSwitch.where("host_id = ?", host_id) ).each
to filter this only to the switches the user is allowed to see..
vm = @reconfigureitems.first | ||
|
||
vm.hardware.guest_devices.order(:device_name => 'asc').each do |guest_device| | ||
lan = Lan.find_by(:id => guest_device.lan_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.
Rbac?
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.
@himdel Is rbac necessary for lans that are already connected to the vm? Should the vm owner be blocked removing a non accessible lan from his vm?
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.
Oh, you're right, here it makes less sense, never mind :).
(But if you wanted to, it would be spelled find_record_with_rbac(Lan, guest_device.lan_id)
.)
vm.processAddSelectedNetwork = function() { | ||
vm.reconfigureModel.vmNetworkAdapters.push( | ||
{ | ||
name: 'to be determined', |
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.
missing i18n? (and for mac)
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.
Good point. I will change it into name: __('to be determined')
@@ -211,6 +291,8 @@ ManageIQ.angular.app.controller('reconfigureFormController', ['$http', '$scope', | |||
add_remove: 'add'}); | |||
vm.resetAddValues(); | |||
vm.updateDisksAddRemove(); | |||
|
|||
vm.cb_disks = vm.reconfigureModel.vmAddDisks.length > 0 || vm.reconfigureModel.vmRemoveDisks.length > 0; |
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 inadvertantly messed with cb_disks. I will remove line 295 and 334 from my PR.
{ | ||
name: 'to be determined', | ||
vlan: vm.reconfigureModel.vLan_requested, | ||
mac: 'not available yet', |
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 will change this into mac: __('not available yet'),
Checked commits https://github.com/kruge002/manageiq-ui-classic/compare/215e9b45bcffea1fa97a8e0a5e0ae1ca96261364~...47ee5232e771e8fb02f46303576c0a481d6528d9 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 **
|
Please retry the build. It timed out which happens ofter lately. |
Please ignore the codeclimate issue: Method reconfigure has a Cognitive Complexity of 6 (exceeds 5 allowed). |
ping @himdel @martinpovolny can you take another look? |
@himdel Is there anything I should add or modify? |
Very sorry for the delay @kruge002 , I think this looks good :) |
@himdel Can this be |
I think so, yes, the UI side should be fine :) (but does need all the dependencies it seems) |
Reconfigure VM: Add / Remove Network Adapters (cherry picked from commit c747bdd)
Gaprindashvili backport details:
|
This PR adds a new feature to the 'Reconfigure VM' webpage: Add / Remove Network Adapters.
This PR is part of a set of PRs:
ManageIQ/vmware_web_service#25
ManageIQ/manageiq-providers-vmware#163
ManageIQ/manageiq#16700
#3121
Issue #3119 describes the lack of support for adding or removing network adapters at the Reconfigure VM webpage. There are some screenshots and a link to the four PRs implementing this new functionality.