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

r/pagerduty_schedule: Don't read back start value for schedule layers. #35

Merged
merged 2 commits into from
Oct 3, 2017

Conversation

heimweh
Copy link
Collaborator

@heimweh heimweh commented Aug 23, 2017

This PR fixes a bug found in pagerduty_schedule (described in #34) related to start.

The PagerDuty API always returns a new start value,
which in turn causes Terraform to get a diff on every plan.

This PR should resolve that, by ignoring whatever value we get back from the API.

@2rs2ts
Copy link

2rs2ts commented Aug 24, 2017

👏 thanks for the super quick response on this issue!

@2rs2ts
Copy link

2rs2ts commented Sep 20, 2017

Is there something holding this back from being merged?

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

Hey! Is there any PagerDuty API docs for this behavior? At a minimum we need to document this behavior in https://www.terraform.io/docs/providers/pagerduty/r/schedule.html . If docs exist, let's link to them.

Also, if the updated/new start value from the API has any kind of significance or nice to know, would it be worth storing that value in a Computed attribute?


Requested change:

  • document behavior in our docs, link to PagerDuty API docs if they exist

Possibly nice to have but not blocker:

  • save the always updating value in a Computed attributed, if it's useful

@2rs2ts
Copy link

2rs2ts commented Sep 26, 2017

The PagerDuty API doesn't really document the resources it exposes or any of their quirks, just the generic API methods available. https://v2.developer.pagerduty.com/docs/endpoints

I poked around the API and I believe it's not that the API always returns a new value, but rather, the start time is getting updated every time terraform updates the schedule. It may actually be that there's a way to prevent that from happening, but I'm not sure right now.

@heimweh
Copy link
Collaborator Author

heimweh commented Sep 28, 2017

@2rs2ts - Your observation is correct. The start time when updating a schedule layer will always be the time when the schedule layer was last updated, also confirmed this with the support team at PagerDuty.

@catsby - Unfortunately this behavior isn't documented anywhere, but I made an effort describing the behavior in the resource documentation. I don't think the updated value we get back from the API is of any real value, but it would in that case make more sense to store that updated value under a different name such as: last_updated. Would you mind taking a look again? many thanks! 🙇

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

Thanks for investigating and documenting 👍

@heimweh heimweh merged commit 2df9991 into PagerDuty:master Oct 3, 2017
@Nowaker
Copy link

Nowaker commented Oct 3, 2017

Nice @heimweh!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants