-
Notifications
You must be signed in to change notification settings - Fork 190
chore: migrate alert_configuration to new SDK #1630
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.
LGTM. just to check if you tried (manually) that plan doesn't change after this change
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.
some details to follow up
apiReq := &matlas.AlertConfiguration{ | ||
EventTypeName: alertConfigPlan.EventType.ValueString(), | ||
apiReq := &admin.GroupAlertsConfig{ | ||
EventTypeName: alertConfigPlan.EventType.ValueStringPointer(), |
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.
curious why SDK is generating this field as optional (string pointer) when it is required for all cases. (does not block PR, just something that could be worth investigating to gain more understanding in sdk 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.
is the "required" info in OpenAPI or it's just how the server behaves?
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.
was observing from the API docs, which reflect the API Spec (https://www.mongodb.com/docs/atlas/reference/api-resources-spec/v2/#tag/Alert-Configurations/operation/createAlertConfiguration)
Let's wait to merge this until we release 1.13.0! see my comment in Jira |
|
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 looking into comments
Description
Migrate alert_configuration to new SDK.
The existing tests already cover all behaviours of the resource, so no tests have been added.
Link to any related issue(s):
https://jira.mongodb.org/browse/INTMDB-1121
Type of change:
Required Checklist:
Further comments