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

Make sure PD services don't get removed by updating PD resource #304

Merged
merged 1 commit into from
Aug 29, 2019
Merged

Make sure PD services don't get removed by updating PD resource #304

merged 1 commit into from
Aug 29, 2019

Conversation

bkabrda
Copy link
Contributor

@bkabrda bkabrda commented Aug 28, 2019

Previously, doing any update on the PD resource would delete the individual service objects. This PR fixes that (at the cost of doing the migration from inline services to individual service objects somewhat harder, but that's a one-time migration process, so shouldn't hurt a lot - I don't see a better way to solve this).

Fixes #299.

@nmuesch
Copy link
Contributor

nmuesch commented Aug 28, 2019

Looks good overall. Just want to verify I understand the issue/solution:

  • User has no services setup under the pd resource but under the service resource
  • User runs TF Apply and everything is created, pd and services
  • User runs TF Apply again, and there is a sensitive field api_token that the API doesn't return so TF assumes its changed and tries to update
  • Since only the original PD resource updated, it would delete all services and not re apply all underlying services since they are part of a separate resource?

While this does fix that issue, I think the underlying issue is that the api_token isn't returned by the API, as expected, but terraform always thinks the pd resource needs to be updated. I think we should add the ignore_changes block to the pagerduty resource docs. Similar to this section - https://www.terraform.io/docs/providers/datadog/r/monitor.html#silencing-by-hand-and-by-downtimes. What do you think?

@bkabrda
Copy link
Contributor Author

bkabrda commented Aug 29, 2019

@nmuesch you're right in all your points, but the third point is not complete - the individual services would get deleted on any change to the pagerduty resource, not just change to the api_token (this is shown in the test, which actually changes subdomain instead of api_key).

I don't consider the api_token doc change to be strictly related to this, so I don't want to get it mixed in this PR, but I think what you're proposing for it makes sense.

@nmuesch
Copy link
Contributor

nmuesch commented Aug 29, 2019

Alright thanks for the confirmation. The only reason I brought up api_token is that it's going to cause every plan to see an update, instead of the individual cases when another field is explicitly updated. I'm happy to do this in a separate PR though.

@bkabrda
Copy link
Contributor Author

bkabrda commented Aug 29, 2019

Thanks for the review, merging.

@bkabrda bkabrda merged commit d8cf584 into DataDog:master Aug 29, 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.

404 error finding datadog_integration_pagerduty_service_object
2 participants