-
Notifications
You must be signed in to change notification settings - Fork 409
[feature] Update Monitors terraform to support draft monitors #3275
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
base: master
Are you sure you want to change the base?
[feature] Update Monitors terraform to support draft monitors #3275
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.
Minor suggestion but approved!
Co-authored-by: domalessi <111786334+domalessi@users.noreply.github.com>
Are we missing the read in |
datadog/resource_datadog_monitor.go
Outdated
m.SetName(d.Get("name").(string)) | ||
m.SetMessage(d.Get("message").(string)) | ||
m.SetOptions(o) | ||
m.SetDraftStatus(d.Get("draft_status").(datadogV1.MonitorDraftStatus)) |
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 assertion correct? Following the monitor type enum, do we need to assert as a string and convert the same way?
m.SetDraftStatus(datadogV1.MonitorDraftStatus(d.Get("draft_status").(string)))
Also present below ln 826
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.
In this case SetDraftStatus is expecting an instance of datadogV1.MonitorDraftStatus
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, right, but does d.Get("draft_status") return an instance of datadogV1.MonitorDraftStatus or a string? Following the other enum example it looked like it returns a string and needed to be converted?
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.
d.Get("draft_status") will return a string that we have to convert to the enum, this should be updated now
@nickpaw Updated updateMonitorState for both providers now leaving it as an optional field |
ValidateDiagFunc: validators.ValidateEnumValue(datadogV1.NewMonitorOptionsNotificationPresetsFromValue), | ||
}, | ||
"draft_status": { | ||
Description: "Indicates whether the monitor is in a draft or published state. When set to `draft`, the monitor appears as Draft and does not send notifications. When set to `published`, the monitor is active, and it evaluates conditions and sends notifications as configured.", |
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.
Can we note that the default value if not configured is published
?
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.
Optional: true, | ||
}, | ||
"draft_status": schema.StringAttribute{ | ||
Description: "The type of the monitor. The mapping from these types to the types found in the Datadog Web UI can be found in the Datadog API [documentation page](https://docs.datadoghq.com/api/v1/monitors/#create-a-monitor). Note: The monitor type cannot be changed after a monitor is created.", |
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.
Description here looks like it was for monitor.type?
Updated monitors terraform to add support for setting draft status as an optional value with it defaulting to published when not set - https://datadoghq.atlassian.net/browse/MA-6686