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

Allow disabling service incident timeouts (take 2) #52

Merged
merged 3 commits into from
Dec 5, 2017

Conversation

pdecat
Copy link
Contributor

@pdecat pdecat commented Nov 8, 2017

Resolves #51 (and #41 for good).

Checklist:

Rationale:

This PR changes auto_resolve_timeout and acknowledgement_timeout fields from int to string type to allow passing a "null" value which means disabled as documented at https://v2.developer.pagerduty.com/v2/page/api-reference#!/Services/post_services

My previous fix in #44 did not work as expected as 0 effectively means 0 second, resulting in incidents to be instantaneously marked as resolved.

To avoid backward incompatible changes to existing terraform configurations, I preserved the default API values of 4h for auto_resolve_timeout and 30 minutes for acknowledgement_timeout.
Note that those are not explicitely stated in the v2 API documentation so I got them from the API v1: https://v1.developer.pagerduty.com/documentation/rest/services/create

I'm preparing a PR for go-pagerduty to remove the local vendor patch.

Acceptance tests results:

make testacc TEST=./pagerduty/ TESTARGS='-run=TestAccPagerDutyService_.*'                                         
==> Checking that code complies with gofmt requirements...                                   
TF_ACC=1 go test ./pagerduty/ -v -run=TestAccPagerDutyService_.* -timeout 120m               
=== RUN   TestAccPagerDutyService_import      
--- PASS: TestAccPagerDutyService_import (11.78s)
=== RUN   TestAccPagerDutyService_Basic
--- PASS: TestAccPagerDutyService_Basic (21.37s)
=== RUN   TestAccPagerDutyService_BasicWithIncidentUrgencyRules
--- PASS: TestAccPagerDutyService_BasicWithIncidentUrgencyRules (15.78s)
=== RUN   TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules
--- PASS: TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules (17.38s)
PASS
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   66.371s

@pdecat
Copy link
Contributor Author

pdecat commented Nov 8, 2017

Expected travis failure given the local vendor patch:

$ make vendor-status
The following packages are missing or modified locally:
	github.com/heimweh/go-pagerduty/pagerduty
Error: status failed for 1 package(s)
make: *** [vendor-status] Error 2
The command "make vendor-status" exited with 2.

@pdecat pdecat changed the title [WIP] Allow disabling service incident timeouts (take 2) Allow disabling service incident timeouts (take 2) Nov 25, 2017
@pdecat pdecat force-pushed the f-disable-service-timeouts-take-2 branch from 4294354 to e9a58e4 Compare November 27, 2017 11:16
@heimweh heimweh self-requested a review December 5, 2017 16:48
@heimweh
Copy link
Collaborator

heimweh commented Dec 5, 2017

Hi @pdecat,

thanks for this PR and the detailed description!

I've ran the acceptance tests as well as tested this manually and can confirm that this is working correctly.

Since the current behavior is broken and you've solved the BC issue and updated the documentation I feel like this should be safe and good to go 👍

Huge thanks for this, is there anything you want to add or can I go ahead and merge this?

Best,

Alexander

@heimweh heimweh added the bug label Dec 5, 2017
@pdecat
Copy link
Contributor Author

pdecat commented Dec 5, 2017

Hi @heimweh,

from my point of view, it is good to merge.

Best regards,
Patrick.

@heimweh heimweh merged commit fcf08ed into PagerDuty:master Dec 5, 2017
@jtsaito
Copy link
Contributor

jtsaito commented Jan 4, 2018

@pdecat Great job!
@heimweh Can we expect a new release with this fix soon?

@pdecat pdecat deleted the f-disable-service-timeouts-take-2 branch July 10, 2018 06:57
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.

Unable to disable acknowledgement_timeout and auto_resolve_timeout
3 participants