PowerVS: Add an only IPI regions list

[hamzy]

In order for the IPI installer to be able to use this module, it needs the ability to have the list of regions to only include supported regions.

[ppc64le-cloud-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hamzy
Once this PR has been reviewed and has the lgtm label, please assign mkumatag for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hamzy]

@mkumatag How about this version?

[mkumatag]

I'm little uncomfortable overriding the variables like Regions, wondering if there any better way to deal with this? may be a better implementation of this utility itself! 🤔

Any idea will be appreciated @Karthik-K-N @dharaneeshvrd

[dharaneeshvrd]

Overall, a bit of refactoring required, added my comments!

[dharaneeshvrd]

Why do we need to do like this? Any reason behind this?
IMHO, we can use the existing map itself with the addition of IPISupported field on each region.

[dharaneeshvrd]

It seems unused

[hamzy]

It is used. Have you searched the code?

[dharaneeshvrd]

Not required IMO, if we just modify the existing Regions map itself.

[dharaneeshvrd]

Suggested change

	func UseIPIRegions () {
	func GetIPIRegions () map[string]Region {

This can be used to filter the IPI regions alone and return it.

[dharaneeshvrd]

Suggested change

	Regions = IPIRegions
	return IPIRegions

[hamzy]

@dharaneeshvrd The reason why I do it this way is so the user can switch between the two lists at any time.

@mkumatag The other way I see to implement this is to alter every API call to add an additional parameter to indicate that only IPI is requested. This would require everyone to change and therefore be unacceptable. Feel free to submit a PR for code you don't have a problem with.

It seems to me that this is a situation where the approver does not work on the IPI project and therefore does not feel the constant pain of having to implement new zones and/or regions.

[mjturek]

Does adding the parameter really mean we'll need to expose it in every API call? Another option is a function that returns a list of keys into the Region list (dal, wdc, mad, etc)

[hamzy]

AFAIK Go does not have a default parameter. I don't even think you can have two different functions with the same name but different number of parameters.

[dharaneeshvrd]

@hamzy I understood what you are trying to do now. The changes will work.
But I am also not that comfortable with overriding variables and creating another instance of map[string]Region.

Please see below approach will work!
With your current design also we need to call UseIPIRegions() to map IPIRegions to Regions var.
How about using some kind of env var to decide on each func to look for IPISupported field.
So we don't need to alter func params and like calling UseIPIRegions() one time, we can set the env var.
With above approach we can avoid the overriding and creating new map.

Another suggestion is if IPI is the major consumer of this utility pkg, then we can keep that env as default to IPIRegions and other consumers like hypershift can keep that env var to false and use it.

[hamzy]

@dharaneeshvrd So you would rather change 11 functions, and all future functions, than one list? Would you agree that this way is more maximally invasive than the previous way?

[dharaneeshvrd]

Agree, it's a lot of change. I think current implementation looks good to me!