-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for systypes w.r.t zones #18
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GunaKKIBM The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -47,8 +50,7 @@ type Region struct { | |||
Description string | |||
VPCRegion string | |||
COSRegion string | |||
Zones []string | |||
SysTypes []string | |||
Zones map[string]SysTypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked in CAPI and openshift, this isn't a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, thanks for verifying.
region.go
Outdated
@@ -287,12 +285,16 @@ func RegionFromZone(zone string) string { | |||
} | |||
|
|||
// AvailableSysTypes returns the default system type for the zone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// AvailableSysTypes returns the default system type for the zone. | |
// AvailableSysTypes returns the supported system type for the zone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
region.go
Outdated
knownZone, ok := Regions[region].Zones[zone] | ||
if !ok { | ||
return nil, fmt.Errorf("unknown zone name provided") | ||
} | ||
return knownZone, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@Karthik-K-N #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. |
Changes LGTM |
The changes here lists available systypes against zones.
Fixes #17