ovirt-networking: using profiles #14991
Conversation
|
@Ladas please review |
|
Shouldn't it be labelled with 'enhancement' instead of 'bug' ? |
| @@ -418,8 +418,8 @@ | |||
| :dvs: true | |||
| :vlans: true | |||
| :method: :allowed_vlans | |||
| :description: vLan | |||
| :required: true | |||
| :description: Virtual Nic Profile | |||
There was a problem hiding this comment.
Shouldn't the 'Virtual Nic Profile' be used as a header as a dependency of the provider highest supported version ?
There was a problem hiding this comment.
Should this be NIC not Nic since it is an acronym?
There was a problem hiding this comment.
done, changed to Network
| rails_logger('allowed_vlans', 1) | ||
| end | ||
|
|
||
| return vlans, hosts | ||
| end | ||
|
|
||
| def load_allowed_vlans(hosts, vlans) | ||
| hosts.each do |h| | ||
| h.lans.each { |l| vlans[l.name] = l.name if @vlan_options[:dvs] || !l.switch.shared? } |
There was a problem hiding this comment.
could you pass @vlan_options as a parameter?
def load_allowed_vlans(hosts, vlans, vlan_options = [])
@agrare ^ this should not break VMware? Not sure if we have specs for this particular case.
There was a problem hiding this comment.
It shouldn't break VMware because we override the whole available_vlans_and_hosts method here
Why did you add back in the if @vlan_options[:dvs]? I removed this in this PR #14292 because we moved all dvs handling to vmware here ManageIQ/manageiq-providers-vmware#25
There was a problem hiding this comment.
Looking at this more I don't think you need to touch this at all, its just moving code around and its just three lines so I don't think it helps readability, unless you're planning on overriding the load_allowed_vlans method in a subclass I think you should just leave it the way it is.
There was a problem hiding this comment.
Ha so I just noticed you did exactly that here https://github.com/ManageIQ/manageiq-providers-ovirt/pull/27/files#diff-9be4bb7e189cb01956b975459128cc04R96 so completely ignore https://github.com/ManageIQ/manageiq/pull/14991/files#r115488788 :)
Still you should leave the unless l.switch.shared? from the original method
AlonaKaplan
force-pushed
the
ovirt_networking_2
branch
from
105d55b
to
706cffc
Compare
AlonaKaplan
force-pushed
the
ovirt_networking_2
branch
from
706cffc
to
2b9eb89
Compare
|
@miq-bot remove_label bug |
|
@miq_bot add_label enhancement |
|
@AlonaKaplan please update the PR message:
According to this commit the new name is 'Network'.
or omit that part completely as it is irrelevant for this specific PR. |
AlonaKaplan
force-pushed
the
ovirt_networking_2
branch
from
2b9eb89
to
058027b
Compare
| load_hosts_networks(hosts, vlans) | ||
| end | ||
|
|
||
| def load_hosts_networks(hosts, vlans) |
There was a problem hiding this comment.
Can you leave this as _vlans instead of _networks for consistency? I don't agree with us using vlans here (I like networks better personally) but the concept is called vlans everywhere else here.
This patch fixes some issues in oVirt vm provision network tab. 1. Changing the 'vLan' label to 'Network' 2. Displaying the vnic profiles instead of the host/hosts networks (only profiles configured in the cluster). 3. Using OvirtSDK4. Changes relevant to version 4 only.
AlonaKaplan
force-pushed
the
ovirt_networking_2
branch
from
058027b
to
7049afa
Compare
|
Checked commit AlonaKaplan@7049afa with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@miq-bot add_label enhancement |
|
@oourfali @AlonaKaplan Do you see any issue with back-porting this PR to the |
ovirt-networking: using profiles (cherry picked from commit e831cbe)
|
As per conversation with @gmcculloug, only taking the changes in Fine backport details: |
ovirt-networking: using profiles (cherry picked from commit e831cbe)
This patch fixes some issues in oVirt vm provision network tab.
profiles configured in the cluster).
Changes relevant to version 4 only.