-
Notifications
You must be signed in to change notification settings - Fork 45
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
Deploymentapi: Add missing GET parameters. #469
Conversation
WithShowSettings(ec.Bool(params.ShowSettings)). | ||
WithConvertLegacyPlans(ec.Bool(params.ConvertLegacyPlans)). | ||
WithShowSystemAlerts(systemAlerts) | ||
if params.ShowInstanceConfigurations { |
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.
Why can't we use the same style as for, e.g. ShowPlans
? E.g.
.WithShowInstanceConfigurations(ec.Bool(params.ShowInstanceConfigurations))
instead of the explicit if
?
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.
The difference is that the if only adds the parameter to the request if it is set to true (assuming the default in the API is false as well). The other parameters will be added to every request.
My reasoning behind it is that it will only add the parameter to the request when it is needed. So if you don't use the new params, nothing will change in the request being made.
Another option would be to make the parameters *bool
and pass them straight in. Then it is still possible in the cloud-sdk-go
api to decide to either set the parameter, or not set it and it will not be added.
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 👍
Implements some new fields in the deployment.get API that weren't supported yet by the SDK.
Description
Related Issues
Motivation and Context
How Has This Been Tested?
Types of Changes
Readiness Checklist