-
Notifications
You must be signed in to change notification settings - Fork 34
CLOUDP-59516: List available matchers for alerts in the Atlas client #70
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.
small nit but in general this is great
mongodbatlas/alert_configurations.go
Outdated
@@ -17,6 +17,7 @@ type AlertConfigurationsService interface { | |||
GetAnAlertConfig(context.Context, string, string) (*AlertConfiguration, *Response, error) | |||
GetOpenAlertsConfig(context.Context, string, string) ([]AlertConfiguration, *Response, error) | |||
List(context.Context, string, *ListOptions) ([]AlertConfiguration, *Response, error) | |||
ListMatcherFields(ctx context.Context) (*[]string, *Response, error) |
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] I wouldn't go for pointers here
ListMatcherFields(ctx context.Context) (*[]string, *Response, error) | |
ListMatcherFields(ctx context.Context) ([]string, *Response, error) |
But if you do it should be []*string
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, @themantissa ready for review on your side
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! @PacoDw and @marinsalinas can one of you look over this please? It seems to have been missed in the work adding support for alert configurations but assume we didn't need for Terraform, if we do we'll now have it so please keep it in mind?
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! Thank you so much 👍
[CLOUDP-59516(https://jira.mongodb.org/browse/CLOUDP-59516)