Fixes #35248 - Show mac address in VmWare info #9957
Conversation
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
| @@ -268,6 +268,10 @@ def show_vm | |||
| end | |||
| attributes = @vm.attributes.deep_symbolize_keys | |||
| attributes[:provider] = @compute_resource.provider | |||
| if attributes[:provider] == "Vmware" | |||
| attributes[:mac_addresses] = @vm.mac_addresses | |||
There was a problem hiding this comment.
why do we need both? which one is the method that has the right value?
There was a problem hiding this comment.
I'm using attributes[:mac] = @vm.mac, for displaying the mac address in hammer output, the same method is used for UI as well.
With attributes[:mac_addresses] = @vm.mac_addresses , I'm overriding the "mac_addresses" param of attributes for the API response which otherwise would be '{}'. I explained why here.
also mac returns the value in the string format eg: "00:50:56:88:11:9e"
and mac_addresses returns it as a hash eg:{"Network Adapter 1"=> "00:50:56:88:11:9e"}
I suppose its better that I update only one value (mac_addresses) but that would need me to format the hash value to a string somewhere in the hammer_cli_foreman plugin
There was a problem hiding this comment.
after the discussion, I looked at other compute resources and there's no good abstraction. Libvirt reports macs for all nics under key "nics" in a hash.
I start leaning towards "get what's available" approach.
FTR, the mac is not among attributes, since it's implemented as a method https://github.com/fog/fog-vsphere/blob/ddc5f494e72b15de375900d0c87afd9fdcde8b9c/lib/fog/vsphere/models/compute/server.rb#L218-L220
I think this is good to go and the additional mac_addresses field can be only useful (today it was empty)
| @@ -268,6 +268,10 @@ def show_vm | |||
| end | |||
| attributes = @vm.attributes.deep_symbolize_keys | |||
| attributes[:provider] = @compute_resource.provider | |||
| if attributes[:provider] == "Vmware" | |||
| attributes[:mac_addresses] = @vm.mac_addresses | |||
There was a problem hiding this comment.
after the discussion, I looked at other compute resources and there's no good abstraction. Libvirt reports macs for all nics under key "nics" in a hash.
I start leaning towards "get what's available" approach.
FTR, the mac is not among attributes, since it's implemented as a method https://github.com/fog/fog-vsphere/blob/ddc5f494e72b15de375900d0c87afd9fdcde8b9c/lib/fog/vsphere/models/compute/server.rb#L218-L220
I think this is good to go and the additional mac_addresses field can be only useful (today it was empty)
|
Btw I don't think this PR is blocked, it's the other way around. API can be extended first, hammer can then rely on the change. |
Yes it's not. Thank you so much @ares , updated the description |
Hammer changes that rely on this PR are: theforeman/hammer-cli-foreman#624