-
Notifications
You must be signed in to change notification settings - Fork 72
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
CLOUDP-69682: Add global API whitelist support #376
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.
Just a couple of nitpicks that are worth addressing, then lgtm
createWhitelist = "Create an access whitelist for Global API Key." | ||
deleteWhitelist = "Delete a database user from Global API Key." | ||
listWhitelist = "List Atlas whitelist entries for Global API Key." | ||
describeEntry = "Return one Global Whitelist Entry using its unique identifier." |
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.
[np] given these are in the package, could just be "create/delete/list/describe"?
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.
they could, I've have issues in other packages of clash so I won't fix this here but feel free to improve on the idea, I even think we should consider getting rid of this file
DeleteGlobalAPIKeyWhitelist(string) error | ||
} | ||
|
||
// GlobalAPIKeyWhitelists encapsulates the logic to manage different cloud providers |
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.
[np] comments
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 change but it is not a blocker. LGTM! Great work! 🚀
const ( | ||
short = "Manage the access whitelist for Global API Key." | ||
createWhitelist = "Create an access whitelist for Global API Key." | ||
deleteWhitelist = "Delete a database user from Global API Key." |
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.
deleteWhitelist = "Delete a database user from Global API Key." | |
deleteWhitelist = "Delete an access whitelist for Global API Key." |
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.
copy/pasta and wrong also in the original, thanks for catching this
@@ -67,7 +67,5 @@ func DeleteBuilder() *cobra.Command { | |||
|
|||
cmd.Flags().StringVar(&opts.OrgID, flag.OrgID, "", usage.OrgID) | |||
|
|||
_ = cmd.MarkFlagRequired(flag.Role) |
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.
drive by fix
@@ -16,6 +16,6 @@ package whitelist | |||
const ( | |||
short = "Manage the access whitelist for your API Key." | |||
createWhitelist = "Create an access whitelist for your API Key." | |||
deleteWhitelist = "Delete a database user from your API Key." |
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.
drive by fix
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
Proposed changes
Jira ticket: CLOUDP-69682
Checklist
make fmt
and formatted my codeFurther comments
List
Describe
Create
Delete