Fixup region label for GPC #93
Conversation
paulajulve
requested review from
jjo,
inkel,
Pokom,
the-it,
nikimanoledaki and
logyball
| switch p.Name() { | ||
| case providerAWS : | ||
| return z[:len(z)-1] | ||
| case providerGCP: | ||
| return z[:len(z)-2] | ||
| default: |
There was a problem hiding this comment.
I think this warrants adding some tests to ensure a regression isn't introduced and some comments with maybe an example so its' clear.
There was a problem hiding this comment.
nevermind, it's already legacy, jjo sneaked this one in yesterday 🤣 lemme give it another spin
| case providerAWS : | ||
| return z[:len(z)-1] | ||
| case providerGCP: | ||
| return z[:len(z)-2] |
There was a problem hiding this comment.
In #93 @jjo introduced a change that allow the user to change the provider name. This would break this switch and the if in line 269. I'd recommend instead doing the following:
| case providerAWS : | |
| return z[:len(z)-1] | |
| case providerGCP: | |
| return z[:len(z)-2] | |
| case aws.ProviderName: | |
| return z[:len(z)-1] | |
| case gcp.ProviderName: | |
| return z[:len(z)-2] |
Note that by doing this and changing the if in 269 accordingly, you can get rid of the new constants in line 15.
|
@paulajulve I believe @jjo did the same in #94, you might want to coordinate with him. |
|
Yup, closing in favour of that one. |
GCP was returning a trailing
-. Fixing up the trim to get the region from the zone depending on each provider.Tried it locally with AWS and GCP on dev and seems to be returning the expected results now.