Add support for service level auto_pause_notifications_parameters #525

pdecat · 2022-06-10T08:53:50Z

This PR adds support for service level auto_pause_notifications_parameters.

Dependencies were updated with go get -u ./... && go mod vendor && go mod tidy to fetch updates from:

Acceptance test results:

make testacc TEST=./pagerduty/ TESTARGS='-run=TestAccPagerDutyService_AutoPauseNotificationsParameters'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./pagerduty/ -v -run=TestAccPagerDutyService_AutoPauseNotificationsParameters -timeout 120m
=== RUN   TestAccPagerDutyService_AutoPauseNotificationsParameters
--- PASS: TestAccPagerDutyService_AutoPauseNotificationsParameters (20.23s)
PASS
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   20.240s

pdecat · 2022-06-10T08:54:39Z

pagerduty/resource_pagerduty_service_test.go

+	service := fmt.Sprintf("tf-%s", acctest.RandString(5))
+
+	resource.Test(t, resource.TestCase{
+		// PreCheck:     func() { testAccPreCheck(t); testAccPreCheckPagerDutyAbility(t, "XXX") }, // FIXME: not sure which ability this feature is depending on https://developer.pagerduty.com/api-reference/9def78ed82b74-list-abilities


I am not sure which ability this feature is depending on https://developer.pagerduty.com/api-reference/9def78ed82b74-list-abilities

pdecat · 2022-06-10T08:56:32Z

Maybe vendored dependencies should be dropped altogether to only rely on go.sum, and avoid having so many files impacted on dependencies updates.

pdecat · 2022-06-10T12:15:47Z

Marked as draft as I missed some important parts in my former commit, and I'm now facing issues.

pdecat · 2022-06-10T13:47:04Z

Submitted heimweh/go-pagerduty#94 to fix issues with JSON serialization. Next time, I'll submit both library and provider PR at the same time as I used to do before.

pdecat · 2022-06-11T13:34:53Z

This is now ready for review.

Acceptance test results:

make testacc TEST=./pagerduty/ TESTARGS='-run=TestAccPagerDutyService_AutoPauseNotificationsParameters'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./pagerduty/ -v -run=TestAccPagerDutyService_AutoPauseNotificationsParameters -timeout 120m
=== RUN   TestAccPagerDutyService_AutoPauseNotificationsParameters
--- PASS: TestAccPagerDutyService_AutoPauseNotificationsParameters (19.31s)
PASS
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   19.323s

pdecat · 2022-06-11T13:37:50Z

pagerduty/resource_pagerduty_service_test.go

+					resource.TestCheckResourceAttr(
+						"pagerduty_service.foo", "auto_pause_notifications_parameters.0.enabled", "false"),
+					resource.TestCheckResourceAttr(
+						"pagerduty_service.foo", "auto_pause_notifications_parameters.0.timeout", "0"),


We get 0 instead of null because that's the default for TypeInt, see hashicorp/terraform-plugin-sdk#90.

Switching to https://github.com/hashicorp/terraform-plugin-framework in the future may help here.

pdecat · 2022-06-20T09:52:26Z

Rebased on master, 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 (12.39s)
=== RUN   TestAccPagerDutyService_Basic
--- PASS: TestAccPagerDutyService_Basic (19.22s)
=== RUN   TestAccPagerDutyService_AlertGrouping
--- PASS: TestAccPagerDutyService_AlertGrouping (16.46s)
=== RUN   TestAccPagerDutyService_AlertContentGrouping
    resource_pagerduty_service_test.go:206: Step 2/2 error: Check failed: Check 8/12 error: pagerduty_service.foo: list or set attribute 'alert_grouping_parameters.0.config' must be checked by element count key (alert_grouping_parameters.0.config.#) or element value keys (e.g. alert_grouping_parameters.0.config.0). Set element value checks should use TestCheckTypeSet functions instead.
--- FAIL: TestAccPagerDutyService_AlertContentGrouping (13.98s)
=== RUN   TestAccPagerDutyService_AutoPauseNotificationsParameters
--- PASS: TestAccPagerDutyService_AutoPauseNotificationsParameters (18.85s)
=== RUN   TestAccPagerDutyService_BasicWithIncidentUrgencyRules
--- PASS: TestAccPagerDutyService_BasicWithIncidentUrgencyRules (21.10s)
=== RUN   TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules
--- PASS: TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules (15.91s)
=== RUN   TestAccPagerDutyService_SupportHoursChange
--- PASS: TestAccPagerDutyService_SupportHoursChange (15.53s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   133.449s
FAIL
make: *** [GNUmakefile:17: testacc] Error 1

The TestAccPagerDutyService_AlertContentGrouping failure is caused by the updated terraform SDK that performs additional checks. I'll submit a fix.

pdecat · 2022-07-23T07:50:14Z

Rebased on master and pulled in heimweh/go-pagerduty@5d470ed, 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 (12.67s)
=== RUN   TestAccPagerDutyService_Basic
--- PASS: TestAccPagerDutyService_Basic (19.62s)
=== RUN   TestAccPagerDutyService_AlertGrouping
--- PASS: TestAccPagerDutyService_AlertGrouping (15.56s)
=== RUN   TestAccPagerDutyService_AlertContentGrouping
    resource_pagerduty_service_test.go:206: Step 2/2 error: Check failed: Check 8/12 error: pagerduty_service.foo: list or set attribute 'alert_grouping_parameters.0.config' must be checked by element count key (alert_grouping_parameters.0.config.#) or element value keys (e.g. alert_grouping_parameters.0.config.0). Set element value checks should use TestCheckTypeSet functions instead.
--- FAIL: TestAccPagerDutyService_AlertContentGrouping (14.85s)
=== RUN   TestAccPagerDutyService_AutoPauseNotificationsParameters
--- PASS: TestAccPagerDutyService_AutoPauseNotificationsParameters (19.02s)
=== RUN   TestAccPagerDutyService_BasicWithIncidentUrgencyRules
--- PASS: TestAccPagerDutyService_BasicWithIncidentUrgencyRules (20.84s)
=== RUN   TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules
--- PASS: TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules (16.19s)
=== RUN   TestAccPagerDutyService_SupportHoursChange
--- PASS: TestAccPagerDutyService_SupportHoursChange (15.84s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   134.603s
FAIL
make: *** [GNUmakefile:17: testacc] Error 1

pdecat · 2022-07-23T08:00:04Z

Hi @imjaroiswebdev, the alert grouping acceptance test is still triggering errors with the new terraform SDK:

# make testacc TEST=./pagerduty/ TESTARGS='-run=TestAccPagerDutyService_AlertContentGrouping'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./pagerduty/ -v -run=TestAccPagerDutyService_AlertContentGrouping -timeout 120m
=== RUN   TestAccPagerDutyService_AlertContentGrouping
    resource_pagerduty_service_test.go:206: Step 2/2 error: Check failed: Check 8/12 error: pagerduty_service.foo: list or set attribute 'alert_grouping_parameters.0.config' must be checked by element count key (alert_grouping_parameters.0.config.#) or element value keys (e.g. alert_grouping_parameters.0.config.0). Set element value checks should use TestCheckTypeSet functions instead.
--- FAIL: TestAccPagerDutyService_AlertContentGrouping (14.83s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   14.839s
FAIL
make: *** [GNUmakefile:17: testacc] Error 1

Changing this to:

git diff
diff --git a/pagerduty/resource_pagerduty_service_test.go b/pagerduty/resource_pagerduty_service_test.go
index a01fa7cd..f17d8101 100644
--- a/pagerduty/resource_pagerduty_service_test.go
+++ b/pagerduty/resource_pagerduty_service_test.go
@@ -254,8 +254,8 @@ func TestAccPagerDutyService_AlertContentGrouping(t *testing.T) {
                                                "pagerduty_service.foo", "alert_creation", "create_alerts_and_incidents"),
                                        resource.TestCheckResourceAttr(
                                                "pagerduty_service.foo", "alert_grouping", "rules"),
-                                       resource.TestCheckNoResourceAttr(
-                                               "pagerduty_service.foo", "alert_grouping_parameters.0.config"),
+                                       resource.TestCheckResourceAttr(
+                                               "pagerduty_service.foo", "alert_grouping_parameters.0.config.#", "0"),
                                        resource.TestCheckNoResourceAttr(
                                                "pagerduty_service.foo", "alert_grouping_parameters.0.type"),
                                        resource.TestCheckResourceAttr(

resolves the issue:

# make testacc TEST=./pagerduty/ TESTARGS='-run=TestAccPagerDutyService_AlertContentGrouping'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./pagerduty/ -v -run=TestAccPagerDutyService_AlertContentGrouping -timeout 120m
=== RUN   TestAccPagerDutyService_AlertContentGrouping
--- PASS: TestAccPagerDutyService_AlertContentGrouping (15.62s)
PASS
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   15.635s

See #525 (comment)

imjaroiswebdev · 2022-08-01T22:09:41Z

Hi @pdecat thank you for this awesome addition to support Auto Pause Notifications Parameters, I've been reviewing it and looks great, but if you don't mind We would prefer to let the module vendoring and tidy for another PR like #532 which you already opened and focus this one in adding this improvement to the Provider. So thinking on that I have rebased and reapplied your commit in a new branch on my fork for you to see how We can do add the change without updating vendors and tidy and continue from there. Please take a look at it in the following branches comparison... https://github.com/pdecat/terraform-provider-pagerduty/compare/auto_pause_notifications_parameters...imjaroiswebdev:terraform-provider-pagerduty:auto_pause_notifications_parameters

The tests you added are also looking very good...

pdecat · 2022-08-02T06:53:36Z

Hi @imjaroiswebdev, updating at least part of the dependencies is required because of heimweh/go-pagerduty@2e1f764

If you don't, go mod vendor complains:

# go get github.com/heimweh/go-pagerduty@5d470ed
go: upgraded github.com/heimweh/go-pagerduty v0.0.0-20220527195341-4e587aa9b58e => v0.0.0-20220722230120-5d470ed7c5fc

# go mod vendor
go: github.com/heimweh/go-pagerduty@v0.0.0-20220722230120-5d470ed7c5fc requires
        go.mongodb.org/mongo-driver@v1.9.1: missing go.sum entry; to add it:
        go mod download go.mongodb.org/mongo-driver
go: github.com/heimweh/go-pagerduty@v0.0.0-20220722230120-5d470ed7c5fc requires
        go.mongodb.org/mongo-driver@v1.9.1: missing go.sum entry; to add it:
        go mod download go.mongodb.org/mongo-driver

pdecat · 2022-08-06T07:40:21Z

Hi @imjaroiswebdev, I've now rebased on master following your go modules updates, and dropped my corresponding commits from this branch.

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 (13.13s)
=== RUN   TestAccPagerDutyService_Basic
--- PASS: TestAccPagerDutyService_Basic (19.95s)
=== RUN   TestAccPagerDutyService_AlertGrouping
--- PASS: TestAccPagerDutyService_AlertGrouping (16.48s)
=== RUN   TestAccPagerDutyService_AlertContentGrouping
--- PASS: TestAccPagerDutyService_AlertContentGrouping (16.27s)
=== RUN   TestAccPagerDutyService_AutoPauseNotificationsParameters
--- PASS: TestAccPagerDutyService_AutoPauseNotificationsParameters (19.37s)
=== RUN   TestAccPagerDutyService_BasicWithIncidentUrgencyRules
--- PASS: TestAccPagerDutyService_BasicWithIncidentUrgencyRules (21.66s)
=== RUN   TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules
--- PASS: TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules (15.50s)
=== RUN   TestAccPagerDutyService_SupportHoursChange
--- PASS: TestAccPagerDutyService_SupportHoursChange (16.09s)
=== RUN   TestAccPagerDutyService_ResponsePlay
--- PASS: TestAccPagerDutyService_ResponsePlay (12.53s)
PASS
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   150.997s

Note: I had to run make fmt to fix an issue with pagerduty/resource_pagerduty_slack_connection.go:

# make test
==> Checking that code complies with gofmt requirements...
gofmt needs running on the following files:
./pagerduty/resource_pagerduty_slack_connection.go
You can use the command: `make fmt` to reformat code.
make: *** [GNUmakefile:32: fmtcheck] Error 1

# make fmt
gofmt -w $(find . -name '*.go' |grep -v vendor)

# make test
==> Checking that code complies with gofmt requirements...
go test -i $(go list ./... |grep -v 'vendor') || exit 1
go: -i flag is deprecated
echo $(go list ./... |grep -v 'vendor') | \
        xargs -t -n4 go test  -timeout=30s -parallel=4
go test '-timeout=30s' '-parallel=4' github.com/terraform-providers/terraform-provider-pagerduty github.com/terraform-providers/terraform-provider-pagerduty/pagerduty
?       github.com/terraform-providers/terraform-provider-pagerduty     [no test files]
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   0.019s

…fications_parameters`

imjaroiswebdev

This a great addition @pdecat thank you so much for your contribution🎉💪🏽, since you have done all the heavy lifting with this excellent PR and all the current and new tests are passing, in the end I only added the final and superficial touches for be able to merged adding a link to the docs which mention the plan restrictions for the PagerDuty account in the documentation you added, on top of that, I removed your inquire about the ability to do the precheck on the test and added the generic test precheck We have, since this configuration can be done by, but the limitation mentioned is related to its use, because of that there is no an explicit ability listed.

pdecat commented Jun 10, 2022

View reviewed changes

pdecat marked this pull request as draft June 10, 2022 12:01

pdecat force-pushed the auto_pause_notifications_parameters branch from a80d024 to 2a5629c Compare June 10, 2022 12:14

pdecat force-pushed the auto_pause_notifications_parameters branch from 2a5629c to 4c79765 Compare June 11, 2022 13:34

pdecat marked this pull request as ready for review June 11, 2022 13:34

pdecat commented Jun 11, 2022

View reviewed changes

pdecat force-pushed the auto_pause_notifications_parameters branch from 4c79765 to f84d64a Compare June 20, 2022 09:45

pdecat mentioned this pull request Jun 20, 2022

Address: Unable to switch off alert grouping on a service #527

Merged

pdecat force-pushed the auto_pause_notifications_parameters branch from f84d64a to ef32613 Compare July 13, 2022 06:25

pdecat mentioned this pull request Jul 13, 2022

Update dependencies #532

Closed

imjaroiswebdev self-requested a review July 22, 2022 15:49

pdecat mentioned this pull request Jul 22, 2022

[Feature Request] - auto_pause_notifications_parameters #547

Closed

pdecat force-pushed the auto_pause_notifications_parameters branch 2 times, most recently from 181ea59 to c22dd71 Compare July 23, 2022 07:43

pdecat force-pushed the auto_pause_notifications_parameters branch 2 times, most recently from b9aea12 to 4f865a7 Compare August 2, 2022 06:57

pdecat added 2 commits August 6, 2022 09:32

Add support for service level auto_pause_notifications_parameters

7a44afe

make fmt

cdb879c

pdecat force-pushed the auto_pause_notifications_parameters branch from 4f865a7 to cdb879c Compare August 6, 2022 07:40

minor tweak to docs and add generic testprecheck for `auto_pause_noti…

1f9ce9d

…fications_parameters`

imjaroiswebdev approved these changes Aug 9, 2022

View reviewed changes

imjaroiswebdev merged commit bb12caa into PagerDuty:master Aug 9, 2022

pdecat deleted the auto_pause_notifications_parameters branch August 9, 2022 06:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for service level auto_pause_notifications_parameters #525

Add support for service level auto_pause_notifications_parameters #525

pdecat commented Jun 10, 2022

pdecat Jun 10, 2022

pdecat commented Jun 10, 2022

pdecat commented Jun 10, 2022

pdecat commented Jun 10, 2022 •

edited

Loading

pdecat commented Jun 11, 2022

pdecat Jun 11, 2022

pdecat commented Jun 20, 2022

pdecat commented Jul 23, 2022

pdecat commented Jul 23, 2022

imjaroiswebdev commented Aug 1, 2022

pdecat commented Aug 2, 2022

pdecat commented Aug 6, 2022 •

edited

Loading

imjaroiswebdev left a comment

Add support for service level auto_pause_notifications_parameters #525

Add support for service level auto_pause_notifications_parameters #525

Conversation

pdecat commented Jun 10, 2022

pdecat Jun 10, 2022

Choose a reason for hiding this comment

pdecat commented Jun 10, 2022

pdecat commented Jun 10, 2022

pdecat commented Jun 10, 2022 • edited Loading

pdecat commented Jun 11, 2022

pdecat Jun 11, 2022

Choose a reason for hiding this comment

pdecat commented Jun 20, 2022

pdecat commented Jul 23, 2022

pdecat commented Jul 23, 2022

imjaroiswebdev commented Aug 1, 2022

pdecat commented Aug 2, 2022

pdecat commented Aug 6, 2022 • edited Loading

imjaroiswebdev left a comment

Choose a reason for hiding this comment

pdecat commented Jun 10, 2022 •

edited

Loading

pdecat commented Aug 6, 2022 •

edited

Loading