-
Notifications
You must be signed in to change notification settings - Fork 86
CLOUDP-62037: Add Labels when Creating/Updating cluster with mongocli Atlas #171
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
Conversation
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.
Sorry if the task was not clear but the requirement is not to expose labels as a flag but to always add a label like this but instead of MongoDB Atlas Terraform Provider
we'll use "MongoDB CLI
couple of gotchas with this is that we should not override existing labels, this may include the fact of someone already using the Infrastructure Tool
key already
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.
Thanks for doing the changes, this looks almost done, a couple of nits here but nothing too major
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.
one small nit but this should get us to ready to merge
|
||
cluster.GroupID = opts.ProjectID() | ||
return cluster, nil | ||
} | ||
|
||
func (opts *atlasClustersCreateOpts) updateLabels(out *atlas.Cluster) { |
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.
[nit] this method doesn't use opts
in its body, so you could make it a generic function like
func (opts *atlasClustersCreateOpts) updateLabels(out *atlas.Cluster) { | |
func updateLabels(out *atlas.Cluster) { |
This also let's you reuse the same function in both create an update
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 thanks for doing this
Proposed changes
Jira ticket: CLOUDP-62037
Checklist
make fmt
and formatted my codeFurther comments
Create Cluster:
Update Cluster: