-
Notifications
You must be signed in to change notification settings - Fork 389
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
Use dates to set downtime interval #113
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.
Overall looks good to me! Just a couple small questions
datadog/resource_datadog_downtime.go
Outdated
Type: schema.TypeInt, | ||
Optional: true, | ||
DiffSuppressFunc: func(k, oldVal, newVal string, d *schema.ResourceData) bool { | ||
_, endDatePresent := d.GetOk("start_date") |
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.
should this be startDatePresent?
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.
Not sure I totally understand. Why suppress the diff if start_date
is there instead of just making it ConflictsWith
?
Is this to satisfy the first case of transitioning from start -> start_date?
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.
If you look at the state file terraform.tfstate
after an apply, you'll see start
is there even if you're using start_date
. This means plan
would always tell you start
has changed even if you're only using start_date
in your .tf
file.
@@ -300,6 +300,49 @@ func testAccCheckDatadogDowntimeExists(n string) resource.TestCheckFunc { | |||
} | |||
} | |||
|
|||
func TestAccDatadogDowntimeDates(t *testing.T) { |
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.
💜
# Datadog API will reject dates in the past so let's ignore `start` and `end` | ||
# arguments during lifecycle | ||
lifecycle { | ||
ignore_changes = ["start", "end"] |
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.
This is likely attached to my initial question around the diffsuppression but not totally clear to me why this needs suppressing?
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.
This is because Datadog API doesn't accept dates in the past at the moment you POST
your request. If you don't ignore start
after the first apply
, terraform always tries to POST
it making the request fail.
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.
I can check it out and test tomorrow (I've been reading the terraform lifecycle docs but I could be misinterpreting this), but in case you know the answer: Does this mean you can't change the end
date here either? I can imagine a case where you'd want to use terraform to extend the end time of an existing downtime.
Additionally, if you wanted to use the same downtime in the future over the same scopes, you wouldn't be able to place the start time in the future as the modification would be ignored by terraform, so a new name for the resource would be required?
As a possible alternative solution, could we validate the start time is not in the past and if it is, use now
or show an 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.
Discussed this offline. The easiest scenario for when one wants to update/extend a downtime would be to comment out this ignore
section, apply a new time, and re place the ignore
section.
Maybe it'd be helpful to update the comment above with:
# Datadog API will reject dates in the past so let's ignore `start` and `end`
# arguments during lifecycle. To update or extend an existing downtime, temporarily
# remove the `ignore` section, apply timestamp changes, and re-apply the `ignore` section.
or something similar?
Would it also be possible/useful to test the case where both are present and ensure we get a conflict error? (Not sure the amount of effort required there) |
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.
Looks good to me. One nit/comment about the docs component, but overall 💯
Use dates to set downtime interval
Setting downtime intervals using Unix timestamps can be very annoying, this PR adds two new arguments
start_date
andend_date
that serve the same purpose astime
anddate
but let users specify a datetime string in RFC3339 format, like:Also suppressed the diff for
active
when the downtime has arecurrence
set. This is to avoid TF complaining aboutactive
changing if ran during downtime window. The implementation is a bit simplistic but should work for the average use case.Fixes #69