Support for systypes w.r.t zones by GunaKKIBM · Pull Request #18 · ppc64le-cloud/powervs-utils

GunaKKIBM · 2025-02-18T13:39:48Z

The changes here lists available systypes against zones.

Fixes #17

mkumatag · 2025-02-18T13:46:34Z

/cc @dharaneeshvrd @Karthik-K-N

Karthik-K-N

Couple of questions

Should we add some tests?
Can we mention somewhere in the file, what is the source of truth for this system types against zone. Is it from PowerVS doc?

Karthik-K-N · 2025-02-19T05:39:31Z

 	COSRegion   string
-	Zones       []string
-	SysTypes    []string
+	Zones       map[string]SysTypes


Not sure we should directly update this Zones data type or add an another filed?

May be if we are planning to update it then once merged lets post this change in channel so consumers are aware of this breaking change.

I have checked in CAPI and openshift, this isn't a breaking change.

cool, thanks for verifying.

Karthik-K-N · 2025-02-19T05:46:15Z

@@ -287,12 +285,16 @@ func RegionFromZone(zone string) string {
 }

 // AvailableSysTypes returns the default system type for the zone.


Suggested change

// AvailableSysTypes returns the default system type for the zone.

// AvailableSysTypes returns the supported system type for the zone.

Karthik-K-N · 2025-02-19T05:48:18Z

+	knownZone, ok := Regions[region].Zones[zone]
+	if !ok {
+		return nil, fmt.Errorf("unknown zone name provided")
+	}
+	return knownZone, nil


Suggested change

knownZone, ok := Regions[region].Zones[zone]

if !ok {

return nil, fmt.Errorf("unknown zone name provided")

}

return knownZone, nil

knownRegion, ok := Regions[region]

if !ok {

return nil, fmt.Errorf("unknown region name provided %s", region)

}

supportedSystemTypes, ok := knownRegion.Zones[zone]

if !ok {

return nil, fmt.Errorf("unknown zone name provided %s", zone)

}

return supportedSystemTypes, nil

Would this make it better?

GunaKKIBM · 2025-02-19T09:59:59Z

Couple of questions

Should we add some tests?

Can we mention somewhere in the file, what is the source of truth for this system types against zone. Is it from PowerVS doc?

@Karthik-K-N
I am not quite sure about this being mentioned in docs.

#17 (comment) - I fetched in from ibmcloud cli as mentioned her.

As for the tests, region_test.go isn't updated. I will add the Unit tests, if that is the expectation.

Karthik-K-N · 2025-02-19T10:38:36Z

Couple of questions

Should we add some tests?

Can we mention somewhere in the file, what is the source of truth for this system types against zone. Is it from PowerVS doc?

@Karthik-K-N I am not quite sure about this being mentioned in docs.

#17 (comment) - I fetched in from ibmcloud cli as mentioned her.

As for the tests, region_test.go isn't updated. I will add the Unit tests, if that is the expectation.

If you can, I think that would be better.

dharaneeshvrd · 2025-02-19T12:54:11Z

Changes LGTM
Ideally the data should come from this https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-ibm-cloud-reg doc, but its not documented there. Paul has populated the data using the ibmloud cli which underneath use this API https://cloud.ibm.com/apidocs/power-cloud#v1-datacenters-getall, for now we can add this in the code.

mkumatag · 2025-04-02T16:05:38Z

@Karthik-K-N @dharaneeshvrd lets review and merge if no further comments

@GunaKKIBM can you please squash the commits?

Karthik-K-N

Changes LGTM,
also can we mention somewhere in the file how these values are retrived may be like as Dharaneesh mentioned here #18 (comment), So the future contributor wont waste time looking for the info!!

Karthik-K-N · 2025-04-03T05:21:46Z

 	"strings"
 )

+// SysTypes denotes the available systypes against individual zones


Suggested change

// SysTypes denotes the available systypes against individual zones

// SysTypes denotes the available system types against individual zones

dharaneeshvrd

lgtm

Karthik-K-N

/lgtm

mkumatag · 2025-04-03T07:58:07Z

+	Zones       map[string]SysTypes
 	VPCZones    []string
 }



Considering so many breaking changes we made to this Region variable, can we make this variable private instead of exposing this to outside world?

Done. I checked if it impacts any other repos, it doesn't

mkumatag

/lgtm

ppc64le-cloud-bot · 2025-04-03T15:28:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharaneeshvrd, GunaKKIBM, mkumatag

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mkumatag]

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

ppc64le-cloud-bot requested review from bkhadars and mkumatag February 18, 2025 13:39

ppc64le-cloud-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 18, 2025

ppc64le-cloud-bot requested review from Karthik-K-N and dharaneeshvrd February 18, 2025 13:46

Karthik-K-N reviewed Feb 19, 2025

View reviewed changes

mkumatag requested a review from Karthik-K-N April 2, 2025 16:05

Karthik-K-N reviewed Apr 3, 2025

View reviewed changes

dharaneeshvrd approved these changes Apr 3, 2025

View reviewed changes

GunaKKIBM force-pushed the Systypes-zones branch from 721e2a1 to 1b5b8c9 Compare April 3, 2025 05:57

Karthik-K-N reviewed Apr 3, 2025

View reviewed changes

ppc64le-cloud-bot assigned Karthik-K-N Apr 3, 2025

ppc64le-cloud-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2025

mkumatag requested changes Apr 3, 2025

View reviewed changes

Support for systypes w.r.t zones

7c21b95

GunaKKIBM force-pushed the Systypes-zones branch from 1b5b8c9 to 7c21b95 Compare April 3, 2025 13:58

ppc64le-cloud-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2025

mkumatag approved these changes Apr 3, 2025

View reviewed changes

ppc64le-cloud-bot assigned mkumatag Apr 3, 2025

ppc64le-cloud-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2025

ppc64le-cloud-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2025

ppc64le-cloud-bot merged commit 219b161 into ppc64le-cloud:main Apr 3, 2025
2 checks passed

Karthik-K-N mentioned this pull request Apr 8, 2025

Add the e1050 System Type kubernetes-sigs/cluster-api-provider-ibmcloud#2167

Closed

GunaKKIBM mentioned this pull request Apr 10, 2025

Add the e1050 System Type kubernetes-sigs/cluster-api-provider-ibmcloud#2304

Merged

		@@ -287,12 +285,16 @@ func RegionFromZone(zone string) string {
		}

		// AvailableSysTypes returns the default system type for the zone.

	// AvailableSysTypes returns the default system type for the zone.
	// AvailableSysTypes returns the supported system type for the zone.

	// SysTypes denotes the available systypes against individual zones
	// SysTypes denotes the available system types against individual zones

Conversation

GunaKKIBM commented Feb 18, 2025 • edited by mkumatag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkumatag commented Feb 18, 2025

Uh oh!

Karthik-K-N left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GunaKKIBM commented Feb 19, 2025

Uh oh!

Karthik-K-N commented Feb 19, 2025

Uh oh!

dharaneeshvrd commented Feb 19, 2025

Uh oh!

mkumatag commented Apr 2, 2025

Uh oh!

Karthik-K-N left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dharaneeshvrd left a comment

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkumatag left a comment

Choose a reason for hiding this comment

Uh oh!

ppc64le-cloud-bot commented Apr 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GunaKKIBM commented Feb 18, 2025 •

edited by mkumatag

Loading