-
Notifications
You must be signed in to change notification settings - Fork 27
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
Make cloud instance that is connected to configurable #148
Conversation
|
||
cloudConfigurationName := spec.CloudConfiguration.Name | ||
switch { | ||
case strings.EqualFold(cloudConfigurationName, "AzurePublic"): |
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.
Please create some Consts for the expected strings
switch { | ||
case strings.EqualFold(cloudConfigurationName, "AzurePublic"): | ||
return cloud.AzurePublic, nil | ||
case strings.EqualFold(cloudConfigurationName, "AzureGovernment"): |
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.
Also maybe its a good idea to have validation for the expected values, before we try to create the client
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.
/lgtm
maybe a unit test for the validation
/hold I think @AndreasBurger wanted to make an update right ? or is it ready to review |
731c1a8
to
468cca2
Compare
468cca2
to
1e5c473
Compare
3c43287
to
1e0b5c4
Compare
I have reverted the latest change and this PR now only uses |
- add consts for known good Azure cloud names - add validation of CloudConfig
1e0b5c4
to
b8ea270
Compare
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.
/lgtm
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.
/lgtm
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.
/lgtm
What this PR does / why we need it:
With this PR the cloud instance to connect to (e.g. Azure Gov Cloud, Azure Public Cloud) can be configured as part of the provider spec.
Which issue(s) this PR fixes:
This PR is a requirement to fulfill gardener/gardener-extension-provider-azure#890
Release note: