Add placement group option to powervs vm provisioning #427

alizapeikes · 2022-10-14T19:18:33Z

connected to ManageIQ/manageiq-ui-classic#8486

agrare · 2022-10-19T17:35:35Z

...odels/manageiq/providers/ibm_cloud/power_virtual_servers/cloud_manager/provision_workflow.rb

+    ar_placement_groups = ar_ems.placement_groups
+    placement_groups = ar_placement_groups&.map { |group| [group['ems_ref'], "#{group['policy']}: #{group['name']}"] }
+    Hash[placement_groups || {}]


You could do a few different things here,

Ensure ar_placement_groups is at least [] like ar_placement_groups = ar_ems.placement_groups || [] then you don't need the &.map

You can .to_h the possibly nil value like ar_placement_groups&.map { |group| }.to_h which would turn nil to {} This is good because you want to use .to_h on the array anyway instead of Hash[placement_groups]

Suggested change

ar_placement_groups = ar_ems.placement_groups

placement_groups = ar_placement_groups&.map { |group| [group['ems_ref'], "#{group['policy']}: #{group['name']}"] }

Hash[placement_groups || {}]

ar_ems.placement_groups&.map { |group| [group['ems_ref'], "#{group['policy']}: #{group['name']}"] }.to_h

Actually when would ar_ems.placement_group be nil ? If it is an association it will at least be an empty assoc

@agrare
ok, so I should just remove the & and ignore the first suggestion?

Yeah you can pass a block to .to_h so even better would be:

ar_ems.placement_groups.to_h { |group| [group.ems_ref, "#{group.policy}: #{group.name}"] }

agrare · 2022-10-19T17:37:19Z

...odels/manageiq/providers/ibm_cloud/power_virtual_servers/cloud_manager/provision_workflow.rb

+  def validate_placement_group(_filed, _values, _dlg, _fld, value)
+    return if value.blank?
+
+    placement_group = ar_ems.placement_groups.find_by(:ems_ref => value)


Recommend either .find_by! which will raise an exception if it can't find the record or handling placement_group being nil if it isn't found by value

...odels/manageiq/providers/ibm_cloud/power_virtual_servers/cloud_manager/provision_workflow.rb

agrare · 2022-10-19T19:59:18Z

content/miq_dialogs/miq_provision_ibm_powervs_dialogs_template.yaml

@@ -17,12 +17,21 @@
      :fields:
        :ip_addr:
          :description: Specify an IPv4 address (applies to first attached network)
-          :required_method: :validate_ip_address
+          :validation_method: :validate_ip_address


This change looks like it is from another PR, make sure to rebase this branch on master

agrare · 2022-10-19T20:01:51Z

...odels/manageiq/providers/ibm_cloud/power_virtual_servers/cloud_manager/provision_workflow.rb

+            else
+              true
+            end
+    return _('Invalid placement group - incompatible colocation policy') unless valid


You actually don't need this redundant return here as ruby automatically returns the last statement

Suggested change

return _('Invalid placement group - incompatible colocation policy') unless valid

_('Invalid placement group - incompatible colocation policy') unless valid

agrare · 2022-10-19T20:02:01Z

...odels/manageiq/providers/ibm_cloud/power_virtual_servers/cloud_manager/provision_workflow.rb

@@ -122,6 +122,12 @@ def allowed_cloud_volumes(_options = {})
    Hash[cloud_volumes || {}]
  end

+  def allowed_placement_groups(_options = {})
+    return {} if ar_ems.nil?
+


Make sure to remove the trailing whitespace here

@agrare
I will remove if you think I should, but just noting that I had originally left the white space in, even with the rubocop warning, to follow the same pattern as similar methods within this file.

manageiq-providers-ibm_cloud/app/models/manageiq/providers/ibm_cloud/power_virtual_servers/cloud_manager/provision_workflow.rb

Lines 57 to 124 in 01085d1

def allowed_instance_type(_options = {})

return {} if ar_ems.nil?

if sap_image?

{

0 => "dedicated"

}

else

{

0 => "capped",

1 => "shared",

2 => "dedicated"

}

end

end

def allowed_sys_type(_options = {})

return {} if ar_ems.nil?

flavor_type = "ManageIQ::Providers::IbmCloud::PowerVirtualServers::CloudManager::#{sap_image? ? 'SAPProfile' : 'SystemType'}"

ar_sys_types = ar_ems.flavors.find_all { |flavor| flavor.type == flavor_type }

sys_types = ar_sys_types&.map&.each_with_index { |sys_type, i| [i, sys_type['name']] }

Hash[sys_types || {}]

end

def allowed_storage_type(_options = {})

return {} if ar_ems.nil?

ar_storage_types = ar_ems.cloud_volume_types

storage_types = ar_storage_types&.map&.each_with_index { |storage_type, i| [i, storage_type['name']] }

Hash[storage_types || none]

end

def allowed_guest_access_key_pairs(_options = {})

return {} if ar_ems.nil?

ar_key_pairs = ar_ems.key_pairs

key_pairs = ar_key_pairs&.map&.with_index(1) { |key_pair, i| [i, key_pair['name']] }

none = [0, 'None']

Hash[key_pairs&.insert(0, none) || none]

end

def allowed_subnets(_options = {})

return {} if ar_ems.nil?

ar_subnets = ar_ems.cloud_subnets

subnets = ar_subnets&.collect { |subnet| [subnet[:ems_ref], subnet[:name]] }

none = ['None', 'None (Must attach to new public network)']

Hash[subnets.unshift(none)]

end

def allowed_cloud_volumes(_options = {})

return {} if ar_ems.nil?

storage_type = values&.dig(:storage_type, 1)

ar_volumes = ar_ems.cloud_volumes.select do |cloud_volume|

cloud_volume['volume_type'] == storage_type &&

(cloud_volume['multi_attachment'] || cloud_volume['status'] == 'available')

end

cloud_volumes = ar_volumes&.map { |cloud_volume| [cloud_volume['ems_ref'], cloud_volume['name']] }

Hash[cloud_volumes || {}]

end

All the other methods in this file are wrong then 😄 I'm guessing they were merged while miq-bot was down since github's diff doesn't show it

Oh, I think there was a miscommunication. The issue isn't the extra newline it was the trailing whitespace.

So:

def allowed_instance_type(_options = {}) return {} if ar_ems.nil? if sap_image?

Is fine, but

def allowed_instance_type(_options = {}) return {} if ar_ems.nil? if sap_image?

Is the issue

agrare · 2022-10-19T20:02:21Z

Just a couple more items then this LGTM

miq-bot · 2022-10-19T20:57:25Z

Checked commit alizapeikes@8ab9e51 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

agrare · 2022-10-19T20:57:31Z

That looks better, recommend using an editor with a setting to strip trailing whitespace as that is what most of our devs use

alizapeikes requested review from agrare, jaywcarman, iv1111, blancett and Fryguy as code owners October 14, 2022 19:18

alizapeikes mentioned this pull request Oct 14, 2022

switch placement group from text-field to pull down field - vm provis… ManageIQ/manageiq-ui-classic#8486

Merged

miq-bot added the wip label Oct 14, 2022

alizapeikes changed the title ~~[WIP] Add placement group option to powervs vm provisioning~~ Add placement group option to powervs vm provisioning Oct 14, 2022

miq-bot removed the wip label Oct 14, 2022

alizapeikes force-pushed the placement-groups branch 2 times, most recently from 1f2c5f2 to b86af82 Compare October 19, 2022 17:05

agrare self-assigned this Oct 19, 2022

agrare added the enhancement New feature or request label Oct 19, 2022

agrare reviewed Oct 19, 2022

View reviewed changes

...odels/manageiq/providers/ibm_cloud/power_virtual_servers/cloud_manager/provision_workflow.rb Show resolved Hide resolved

alizapeikes force-pushed the placement-groups branch from b86af82 to 01085d1 Compare October 19, 2022 19:54

agrare reviewed Oct 19, 2022

View reviewed changes

alizapeikes force-pushed the placement-groups branch from 01085d1 to 2e8656c Compare October 19, 2022 20:33

agrare approved these changes Oct 19, 2022

View reviewed changes

Add placement group option to powervs vm provisioning

8ab9e51

alizapeikes force-pushed the placement-groups branch from 528dac2 to 8ab9e51 Compare October 19, 2022 20:55

agrare merged commit 1d39bf8 into ManageIQ:master Oct 19, 2022

agrare mentioned this pull request Sep 14, 2023

[Petrosian] IBM Cloud PowerVS Provider Enhancements #425

Closed

3 tasks

Fryguy added this to the Petrosian milestone Sep 14, 2023

Fryguy added this to Petrosian in Roadmap Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add placement group option to powervs vm provisioning #427

Add placement group option to powervs vm provisioning #427

alizapeikes commented Oct 14, 2022 •

edited

agrare Oct 19, 2022

agrare Oct 19, 2022

alizapeikes Oct 19, 2022 •

edited

agrare Oct 19, 2022

agrare Oct 19, 2022

agrare Oct 19, 2022

agrare Oct 19, 2022

agrare Oct 19, 2022

alizapeikes Oct 19, 2022

agrare Oct 19, 2022

alizapeikes Oct 19, 2022 •

edited

agrare Oct 19, 2022

agrare commented Oct 19, 2022

miq-bot commented Oct 19, 2022

agrare commented Oct 19, 2022

	return _('Invalid placement group - incompatible colocation policy') unless valid
	_('Invalid placement group - incompatible colocation policy') unless valid

	def allowed_instance_type(_options = {})
	return {} if ar_ems.nil?

	if sap_image?
	{
	0 => "dedicated"
	}
	else
	{
	0 => "capped",
	1 => "shared",
	2 => "dedicated"
	}
	end
	end

	def allowed_sys_type(_options = {})
	return {} if ar_ems.nil?

	flavor_type = "ManageIQ::Providers::IbmCloud::PowerVirtualServers::CloudManager::#{sap_image? ? 'SAPProfile' : 'SystemType'}"

	ar_sys_types = ar_ems.flavors.find_all { \|flavor\| flavor.type == flavor_type }

	sys_types = ar_sys_types&.map&.each_with_index { \|sys_type, i\| [i, sys_type['name']] }
	Hash[sys_types \|\| {}]
	end

	def allowed_storage_type(_options = {})
	return {} if ar_ems.nil?

	ar_storage_types = ar_ems.cloud_volume_types
	storage_types = ar_storage_types&.map&.each_with_index { \|storage_type, i\| [i, storage_type['name']] }
	Hash[storage_types \|\| none]
	end

	def allowed_guest_access_key_pairs(_options = {})
	return {} if ar_ems.nil?

	ar_key_pairs = ar_ems.key_pairs
	key_pairs = ar_key_pairs&.map&.with_index(1) { \|key_pair, i\| [i, key_pair['name']] }
	none = [0, 'None']
	Hash[key_pairs&.insert(0, none) \|\| none]
	end

	def allowed_subnets(_options = {})
	return {} if ar_ems.nil?

	ar_subnets = ar_ems.cloud_subnets
	subnets = ar_subnets&.collect { \|subnet\| [subnet[:ems_ref], subnet[:name]] }
	none = ['None', 'None (Must attach to new public network)']
	Hash[subnets.unshift(none)]
	end

	def allowed_cloud_volumes(_options = {})
	return {} if ar_ems.nil?

	storage_type = values&.dig(:storage_type, 1)

	ar_volumes = ar_ems.cloud_volumes.select do \|cloud_volume\|
	cloud_volume['volume_type'] == storage_type &&
	(cloud_volume['multi_attachment'] \|\| cloud_volume['status'] == 'available')
	end

	cloud_volumes = ar_volumes&.map { \|cloud_volume\| [cloud_volume['ems_ref'], cloud_volume['name']] }

	Hash[cloud_volumes \|\| {}]
	end

Add placement group option to powervs vm provisioning #427

Add placement group option to powervs vm provisioning #427

Conversation

alizapeikes commented Oct 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alizapeikes Oct 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alizapeikes Oct 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Oct 19, 2022

miq-bot commented Oct 19, 2022

agrare commented Oct 19, 2022

alizapeikes commented Oct 14, 2022 •

edited

alizapeikes Oct 19, 2022 •

edited

alizapeikes Oct 19, 2022 •

edited