Skip to content
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

Deprecate Silence attribute and Fix multiple terraform runs #221

Merged
merged 15 commits into from
Jun 11, 2019

Conversation

nmuesch
Copy link
Contributor

@nmuesch nmuesch commented May 30, 2019

This PR looks to accomplish a few things surrounding the silence attribute of monitor resources.

  • Add a deprecation notice that we'll stop supporting the silenced param soon. (In a couple Major versions of the Provider as outlined by the best practices doc - https://www.terraform.io/docs/extend/best-practices/deprecations.html)
  • Add the ability to specify a -1 on any scope of the silenced attribute to unmute that scope. This will make a separate API call to unmute those scopes (Only on updates, newly created monitors shouldn't be muted :) )
  • Fix an issue where running terraform apply would detect a diff on the silence scope if the timestamp were in the past. This is due to the Datadog Monitor API not returning scopes with timestamps in the past in the API response. So the monitor resource will detect this and leave the state unchanged.
  • Add some tests 🎉

@nmuesch nmuesch changed the title Deprecate Silence attribute and Fix multiple state runs Deprecate Silence attribute and Fix multiple terraform runs May 30, 2019
Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So overall this is a real improvement, except one scenario that I pointed in one of the comments. Whenever you fix that, please consider adding some tests for this, since it's somewhat unintuitive (at least for me) how this works.

datadog/resource_datadog_monitor.go Outdated Show resolved Hide resolved
datadog/resource_datadog_monitor.go Outdated Show resolved Hide resolved
@ghost ghost added size/M and removed size/S labels May 31, 2019
@ghost ghost added size/S and removed size/M labels Jun 3, 2019
@ghost ghost added size/M and removed size/S labels Jun 6, 2019
Type: schema.TypeMap,
Optional: true,
Elem: schema.TypeInt,
Deprecated: "use Downtime Resource instead",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ghost ghost added size/L and removed size/M labels Jun 10, 2019
Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, nice job! 👍

I only have one minor nit in terms of test names - if you could name them TestAccDatadogMonitor_Silenced*, it would be easier to run all tests related to silenced with -run TestAccDatadogMonitor_Silenced when running go test. I think it would be more practical, but I don't mean to block this PR getting merged on it.

Also, please squash the commits that you did to not pollute project history. Thanks!

@nmuesch
Copy link
Contributor Author

nmuesch commented Jun 11, 2019

Good idea, just changed the test function names based on your suggestion. I'll do a squash and merge if thats alright with you.

@bkabrda
Copy link
Contributor

bkabrda commented Jun 11, 2019

Yup, go ahead with squashing and merging 👍

@gzussa
Copy link
Contributor

gzussa commented Jun 11, 2019

Great stuff :)

jbenais pushed a commit to jbenais/terraform-provider-datadog that referenced this pull request Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants