-
Notifications
You must be signed in to change notification settings - Fork 381
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
Add default of 10m for monitor no data timeframe #212
Conversation
@@ -132,6 +132,7 @@ func resourceDatadogMonitor() *schema.Resource { | |||
"no_data_timeframe": { | |||
Type: schema.TypeInt, | |||
Optional: true, | |||
Default: 10, |
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.
We should use DefaultFunc
and only set this when notify_no_data
is true
, I think.
Also, we could actually try parsing out the evaluation timeframe from the query and set it to the recommended values from the docs [1] - let's fall back to 10 minutes if fail to parse out the evaluation timeframe.
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.
As discussed offline a bit, lets go with a generically default 10 minutes and just add a note that it should be set to 2x the evaluation window. We can tackle something more complex if its needed later 😄
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.
Actually, the docs have a pretty clear note that the timeframe, if specified, should be at least 2x the eval window. Do you think this is good to be merged?
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.
Yeah, I'm ok with merging this 👍
@@ -132,6 +132,7 @@ func resourceDatadogMonitor() *schema.Resource { | |||
"no_data_timeframe": { | |||
Type: schema.TypeInt, | |||
Optional: true, | |||
Default: 10, |
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.
Yeah, I'm ok with merging this 👍
Add default of 10m for monitor no data timeframe
Adds a default of 10 minutes for the no_data_timeframe to be used instead of the 0 value the provider currently emits.
Ultimately looks to fix #206