Fix alert_grouping and alert_grouping_timeout conflicting with alert_grouping_parameters #377

pdecat · 2021-08-20T10:33:07Z

This PR resolves issues with alert_grouping and alert_grouping_timeout which are mostly visible since the introduction of alert_grouping_parameters in #342.

Indeed alert_grouping and alert_grouping_timeout are conflicting with alert_grouping_parameters that takes precedence over them.

These issues were not apparent when running acceptance tests from master because the terraform state was not updated during service update API calls so it blindly took values from terraform configuration.

Example update API call when running TestAccPagerDutyService_AlertGrouping with current master without changes from this PR:

2021/08/20 12:48:29 [INFO] Updating PagerDuty service PR3OBPD
2021/08/20 12:48:29 [DEBUG] PagerDuty - Preparing PUT request to /services/PR3OBPD with body: {"acknowledgement_timeout":null,"alert_grouping":null,"auto_resolve_timeout":null,"service":{"acknowledgement_timeout":1800,"alert_creation":"create_alerts_and_incidents","alert_grouping":"intelligent","alert_grouping_timeout":1900,"alert_grouping_parameters":{"config":{"timeout":0,"aggregate":""}},"auto_resolve_timeout":1800,"description":"foo","escalation_policy":{"id":"PYCXN93","type":"escalation_policy_reference"},"incident_urgency_rule":{"type":"constant","urgency":"high"},"name":"tf-7di97"}}
2021/08/20 12:48:29 [DEBUG] PagerDuty API Request Details:
---[ REQUEST ]---------------------------------------
PUT /services/PR3OBPD HTTP/1.1
Host: api.pagerduty.com
User-Agent: heimweh/go-pagerduty(terraform)
Content-Length: 503
Accept: application/vnd.pagerduty+json;version=2
Authorization: Token token=******
Content-Type: application/json
Accept-Encoding: gzip

{
 "acknowledgement_timeout": null,
 "alert_grouping": null,
 "auto_resolve_timeout": null,
 "service": {
  "acknowledgement_timeout": 1800,
  "alert_creation": "create_alerts_and_incidents",
  "alert_grouping": "intelligent",
  "alert_grouping_timeout": 1900,
  "alert_grouping_parameters": {
   "config": {
    "timeout": 0,
    "aggregate": ""
   }
  },
  "auto_resolve_timeout": 1800,
  "description": "foo",
  "escalation_policy": {
   "id": "PYCXN93",
   "type": "escalation_policy_reference"
  },
  "incident_urgency_rule": {
   "type": "constant",
   "urgency": "high"
  },
  "name": "tf-7di97"
 }
}

-----------------------------------------------------
2021/08/20 12:48:30 [DEBUG] PagerDuty API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 200 OK
Access-Control-Allow-Headers: Authorization, Content-Type, AuthorizationOauth, X-EARLY-ACCESS
Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: 
Access-Control-Max-Age: 1728000
Cache-Control: max-age=0, private, must-revalidate
Content-Type: application/json
Date: Fri, 20 Aug 2021 10:48:30 GMT
Etag: W/"aa712055b24fe69980922852b70ee91b"
Feature-Policy: accelerometer 'none'; camera 'none'; geolocation 'none'; gyroscope 'none'; magnetometer 'none'; microphone 'none'; payment 'none'; usb 'none'
Referrer-Policy: strict-origin-when-cross-origin
Server: nginx
Strict-Transport-Security: max-age=31536000; includeSubDomains
Vary: Accept-Encoding
X-Content-Type-Options: nosniff
X-Request-Id: 22e0db08bc723251612b3aabe8eb18f8
X-Xss-Protection: 1; mode=block

{
 "service": {
  "id": "PR3OBPD",
  "name": "tf-7di97",
  "description": "foo",
  "created_at": "2021-08-20T12:48:25+02:00",
  "updated_at": "2021-08-20T12:48:30+02:00",
  "status": "active",
  "teams": [],
  "alert_creation": "create_alerts_and_incidents",
  "addons": [],
  "scheduled_actions": [],
  "support_hours": null,
  "last_incident_timestamp": null,
  "escalation_policy": {
   "id": "PYCXN93",
   "type": "escalation_policy_reference",
   "summary": "tf-grexx",
   "self": "https://api.pagerduty.com/escalation_policies/PYCXN93",
   "html_url": "https://dev-claranet.pagerduty.com/escalation_policies/PYCXN93"
  },
  "incident_urgency_rule": {
   "type": "constant",
   "urgency": "high"
  },
  "acknowledgement_timeout": 1800,
  "auto_resolve_timeout": 1800,
  "alert_grouping": null,
  "alert_grouping_timeout": null,
  "alert_grouping_parameters": {
   "type": null,
   "config": null
  },
  "integrations": [],
  "response_play": null,
  "type": "service",
  "summary": "tf-7di97",
  "self": "https://api.pagerduty.com/services/PR3OBPD",
  "html_url": "https://dev-claranet.pagerduty.com/service-directory/PR3OBPD"
 }
}
-----------------------------------------------------
2021/08/20 12:48:30 [DEBUG] provider has no plugin.Client
2021/08/20 12:48:30 [DEBUG] New state was assigned lineage "a816f620-f7e7-961e-9bfa-a3120d33e4dd"

Note how the update of alert_grouping to intelligent is ignored by the API.

Example update API call when running TestAccPagerDutyService_AlertGrouping with changes from this PR:

2021/08/20 12:42:06 [INFO] Updating PagerDuty service P8BSLC3
2021/08/20 12:42:06 [DEBUG] PagerDuty - Preparing PUT request to /services/P8BSLC3 with body: {"acknowledgement_timeout":null,"alert_grouping":null,"auto_resolve_timeout":null,"service":{"acknowledgement_timeout":1800,"alert_creation":"create_alerts_and_incidents","alert_grouping":"intelligent","alert_grouping_timeout":1900,"auto_resolve_timeout":1800,"description":"foo","escalation_policy":{"id":"PEQQG4Z","type":"escalation_policy_reference"},"incident_urgency_rule":{"type":"constant","urgency":"high"},"name":"tf-hnrbp"}}
2021/08/20 12:42:06 [DEBUG] PagerDuty API Request Details:
---[ REQUEST ]---------------------------------------
PUT /services/P8BSLC3 HTTP/1.1
Host: api.pagerduty.com
User-Agent: heimweh/go-pagerduty(terraform)
Content-Length: 435
Accept: application/vnd.pagerduty+json;version=2
Authorization: Token token=******
Content-Type: application/json
Accept-Encoding: gzip

{
 "acknowledgement_timeout": null,
 "alert_grouping": null,
 "auto_resolve_timeout": null,
 "service": {
  "acknowledgement_timeout": 1800,
  "alert_creation": "create_alerts_and_incidents",
  "alert_grouping": "intelligent",
  "alert_grouping_timeout": 1900,
  "auto_resolve_timeout": 1800,
  "description": "foo",
  "escalation_policy": {
   "id": "PEQQG4Z",
   "type": "escalation_policy_reference"
  },
  "incident_urgency_rule": {
   "type": "constant",
   "urgency": "high"
  },
  "name": "tf-hnrbp"
 }
}

-----------------------------------------------------
2021/08/20 12:42:07 [DEBUG] PagerDuty API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 200 OK
Access-Control-Allow-Headers: Authorization, Content-Type, AuthorizationOauth, X-EARLY-ACCESS
Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: 
Access-Control-Max-Age: 1728000
Cache-Control: max-age=0, private, must-revalidate
Content-Type: application/json
Date: Fri, 20 Aug 2021 10:42:07 GMT
Etag: W/"996de9ebaf5040cc3a13171e7b7b489a"
Feature-Policy: accelerometer 'none'; camera 'none'; geolocation 'none'; gyroscope 'none'; magnetometer 'none'; microphone 'none'; payment 'none'; usb 'none'
Referrer-Policy: strict-origin-when-cross-origin
Server: nginx
Strict-Transport-Security: max-age=31536000; includeSubDomains
Vary: Accept-Encoding
X-Content-Type-Options: nosniff
X-Request-Id: 8da2a0631ba6de47b5a6ff3da67ebb4c
X-Xss-Protection: 1; mode=block

{
 "service": {
  "id": "P8BSLC3",
  "name": "tf-hnrbp",
  "description": "foo",
  "created_at": "2021-08-20T12:42:03+02:00",
  "updated_at": "2021-08-20T12:42:07+02:00",
  "status": "active",
  "teams": [],
  "alert_creation": "create_alerts_and_incidents",
  "addons": [],
  "scheduled_actions": [],
  "support_hours": null,
  "last_incident_timestamp": null,
  "escalation_policy": {
   "id": "PEQQG4Z",
   "type": "escalation_policy_reference",
   "summary": "tf-b41xx",
   "self": "https://api.pagerduty.com/escalation_policies/PEQQG4Z",
   "html_url": "https://dev-claranet.pagerduty.com/escalation_policies/PEQQG4Z"
  },
  "incident_urgency_rule": {
   "type": "constant",
   "urgency": "high"
  },
  "acknowledgement_timeout": 1800,
  "auto_resolve_timeout": 1800,
  "alert_grouping": "intelligent",
  "alert_grouping_timeout": 1900,
  "alert_grouping_parameters": {
   "type": "intelligent",
   "config": null
  },
  "integrations": [],
  "response_play": null,
  "type": "service",
  "summary": "tf-hnrbp",
  "self": "https://api.pagerduty.com/services/P8BSLC3",
  "html_url": "https://dev-claranet.pagerduty.com/service-directory/P8BSLC3"
 }
}
-----------------------------------------------------
2021/08/20 12:42:07 [DEBUG] provider has no plugin.Client
2021/08/20 12:42:07 [DEBUG] New state was assigned lineage "94f7405b-332f-b777-03ef-a50c76bbc4a6"

Note: alert_grouping and alert_grouping_timeout were documented as deprecated in the provider documentation (but that's not explicitly documented in the API), so this PR marks them conflicting with alert_grouping_parameters which is especially important as the latter takes precedence over them.

pdecat · 2021-08-20T10:46:26Z

pagerduty/resource_pagerduty_service.go

 			},
 			"alert_grouping_timeout": {
-				Type:     schema.TypeInt,
-				Optional: true,
+				Type:          schema.TypeString,


alert_grouping_timeout must be a string to be able to set it to "null" at https://github.com/PagerDuty/terraform-provider-pagerduty/pull/377/files#diff-ef1847adc67b2e7a9ba18b9c2c36d3a602b5b5938936a3841e7e96ed9b62f048L391

Note: this removed line is just moved to https://github.com/PagerDuty/terraform-provider-pagerduty/pull/377/files#diff-ef1847adc67b2e7a9ba18b9c2c36d3a602b5b5938936a3841e7e96ed9b62f048R443

pdecat · 2021-08-20T10:59:42Z

pagerduty/resource_pagerduty_service.go

 	}
-	// Using GetOkExists to allow for alert_grouping_timeout to be set to 0 if needed.
-	if attr, ok := d.GetOkExists("alert_grouping_timeout"); ok {


With terraform-plugin-sdk v1.7.0 GetOkExists is documented as being only useful for boolean attributes:

// GetOkExists returns the data for a given key and whether or not the key // has been set to a non-zero value. This is only useful for determining // if boolean attributes have been set, if they are Optional but do not // have a Default value. // // This is nearly the same function as GetOk, yet it does not check // for the zero value of the attribute's type. This allows for attributes // without a default, to fully check for a literal assignment, regardless // of the zero-value for that type. // This should only be used if absolutely required/needed.

https://github.com/hashicorp/terraform-plugin-sdk/blob/v1.7.0/helper/schema/resource_data.go#L111-L120

Since terraform-plugin-sdk v1.9.0, it is documented as deprecated as it leads to undefined behaviors:

// GetOkExists can check if TypeBool attributes that are Optional with // no Default value have been set. // // Deprecated: usage is discouraged due to undefined behaviors and may be // removed in a future version of the SDK

https://github.com/hashicorp/terraform-plugin-sdk/blob/v1.9.0/helper/schema/resource_data.go#L111-L115
https://github.com/hashicorp/terraform-plugin-sdk/blob/v1-maint/CHANGELOG.md#190-march-26-2020
hashicorp/terraform-plugin-sdk#350

Note: as this PR converts the attribute from int to string like the other similar attributes (acknowledgement_timeout and auto_resolve_timeout), the ability to set it to 0 is preserved.

pdecat · 2021-09-10T08:46:35Z

Rebased on master, @stmcallister PTAL.

stmcallister · 2021-10-07T01:41:24Z

@pdecat Amazing work on this and the other related PRs! Thank you so much! On this one I'm still getting the following error

TF_ACC=1 go test -run "TestAccPagerDutyService_AlertGrouping" ./pagerduty -v -timeout 120m
=== RUN   TestAccPagerDutyService_AlertGrouping
    resource_pagerduty_service_test.go:137: Step 2/2 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
        stdout
        
        
        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # pagerduty_service.foo will be updated in-place
          ~ resource "pagerduty_service" "foo" {
              ~ alert_grouping          = "time" -> "intelligent"
              ~ alert_grouping_timeout  = "1800" -> "1900"
                id                      = "PQ6RH6C"
                name                    = "tf-8z8ch"
                # (8 unchanged attributes hidden)
        
        
                # (2 unchanged blocks hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccPagerDutyService_AlertGrouping (15.07s)

pdecat · 2021-10-07T06:06:09Z

Weird, it is working fine for me:

# git log -n 1
commit 400e808d049de12b0f30eb8f3a65500c63a3aad7 (HEAD -> fix_alert_grouping, origin/fix_alert_grouping)
Author: Patrick Decat <pdecat@******.com>
Date:   Fri Aug 20 12:41:03 2021 +0200

    Remove redundant call to GET /services/id API as everything is already available in POST /services/id response

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

PS: it would awesome to get acceptance tests running in CI

…ull"`

…t_grouping_parameters is not defined as omitempty only works if the value is nil, not empty

…ng and AlertGroupingTimeout which are apparently deprecated (that's not explicitly documented in the API)

…d and mark them conflicting with `alert_grouping_parameters` that takes precedence over them

…rs if it is defined in configuration

…y available in POST /services/id response

pdecat · 2021-10-07T06:07:11Z

Rebased on master.

pdecat · 2021-10-07T06:08:21Z

# git log -n 1
commit e3d3b7f859c1d0851d858f46d3684c49ce95f2aa (HEAD -> fix_alert_grouping, origin/fix_alert_grouping)
Author: Patrick Decat <pdecat@******.com>
Date:   Thu Oct 7 08:06:58 2021 +0200

    make fmt

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

pdecat · 2021-10-07T06:17:54Z

@stmcallister Is the intelligent alert grouping feature enabled on the account you are testing against?

This feature is in preview: https://support.pagerduty.com/docs/preview-intelligent-alert-grouping

@pdecat Amazing work on this and the other related PRs! Thank you so much! On this one I'm still getting the following error

TF_ACC=1 go test -run "TestAccPagerDutyService_AlertGrouping" ./pagerduty -v -timeout 120m
=== RUN   TestAccPagerDutyService_AlertGrouping
    resource_pagerduty_service_test.go:137: Step 2/2 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
        stdout
        
        
        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # pagerduty_service.foo will be updated in-place
          ~ resource "pagerduty_service" "foo" {
              ~ alert_grouping          = "time" -> "intelligent"
              ~ alert_grouping_timeout  = "1800" -> "1900"
                id                      = "PQ6RH6C"
                name                    = "tf-8z8ch"
                # (8 unchanged attributes hidden)
        
        
                # (2 unchanged blocks hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccPagerDutyService_AlertGrouping (15.07s)

pdecat · 2021-10-07T06:22:09Z

@stmcallister Is the intelligent alert grouping feature enabled on the account you are testing against?

This can be checked with the GET abilities API endpoint which returns preview_intelligent_alert_grouping when the feature is enabled:

https://developer.pagerduty.com/api-reference/reference/REST/openapiv3.json/paths/~1abilities/get

Maybe the acceptance test should check this endpoint automatically and skip if the feature is not enabled.

stmcallister · 2021-10-07T15:59:31Z

@pdecat I do have preview_intelligent_alert_grouping enabled on the account. 🤔

stmcallister

Upon further investigation I did not have this branch properly pulled down to my local repo. 🤦 Once I got everything merged correctly the tests worked beautifully. Thank you for updating this we'll need to make a note in the Changelog that this will technically a breaking change by changing the field from an int to a string.

pdecat commented Aug 20, 2021

View reviewed changes

pdecat mentioned this pull request Aug 20, 2021

Upgrade Terraform Plugin SDK to v2 #375

Merged

pdecat force-pushed the fix_alert_grouping branch from b42cdfa to ee8f976 Compare September 10, 2021 08:05

pdecat force-pushed the fix_alert_grouping branch from ee8f976 to 400e808 Compare September 28, 2021 06:51

pdecat added 8 commits October 7, 2021 08:06

Update state from service update API call response to detect issues

70cf348

alert_grouping_timeout must be a string to be able to set it to `"n…

5933e01

…ull"`

Do not set service.AlertGroupingParameters to an empty struct if aler…

852347b

…t_grouping_parameters is not defined as omitempty only works if the value is nil, not empty

Clear AlertGroupingParameters as it takes precedence over AlertGroupi…

6ba5890

…ng and AlertGroupingTimeout which are apparently deprecated (that's not explicitly documented in the API)

Actually mark alert_grouping and alert_grouping_timeout deprecate…

726e83a

…d and mark them conflicting with `alert_grouping_parameters` that takes precedence over them

Only store service.AlertGroupingParameters in alert_grouping_paramete…

f2a1b13

…rs if it is defined in configuration

Remove redundant call to GET /services/id API as everything is alread…

b5a49eb

…y available in POST /services/id response

make fmt

e3d3b7f

pdecat force-pushed the fix_alert_grouping branch from 400e808 to e3d3b7f Compare October 7, 2021 06:07

stmcallister approved these changes Oct 8, 2021

View reviewed changes

Merge branch 'master' into fix_alert_grouping

1753290

stmcallister merged commit 7b9df94 into PagerDuty:master Oct 8, 2021

pdecat deleted the fix_alert_grouping branch October 8, 2021 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix alert_grouping and alert_grouping_timeout conflicting with alert_grouping_parameters #377

Fix alert_grouping and alert_grouping_timeout conflicting with alert_grouping_parameters #377

pdecat commented Aug 20, 2021 •

edited

Loading

pdecat Aug 20, 2021

pdecat Aug 20, 2021

pdecat Aug 20, 2021

pdecat commented Sep 10, 2021

stmcallister commented Oct 7, 2021

pdecat commented Oct 7, 2021 •

edited

Loading

pdecat commented Oct 7, 2021

pdecat commented Oct 7, 2021 •

edited

Loading

pdecat commented Oct 7, 2021 •

edited

Loading

pdecat commented Oct 7, 2021

stmcallister commented Oct 7, 2021

stmcallister left a comment

Fix alert_grouping and alert_grouping_timeout conflicting with alert_grouping_parameters #377

Fix alert_grouping and alert_grouping_timeout conflicting with alert_grouping_parameters #377

Conversation

pdecat commented Aug 20, 2021 • edited Loading

pdecat Aug 20, 2021

Choose a reason for hiding this comment

pdecat Aug 20, 2021

Choose a reason for hiding this comment

pdecat Aug 20, 2021

Choose a reason for hiding this comment

pdecat commented Sep 10, 2021

stmcallister commented Oct 7, 2021

pdecat commented Oct 7, 2021 • edited Loading

pdecat commented Oct 7, 2021

pdecat commented Oct 7, 2021 • edited Loading

pdecat commented Oct 7, 2021 • edited Loading

pdecat commented Oct 7, 2021

stmcallister commented Oct 7, 2021

stmcallister left a comment

Choose a reason for hiding this comment

pdecat commented Aug 20, 2021 •

edited

Loading

pdecat commented Oct 7, 2021 •

edited

Loading

pdecat commented Oct 7, 2021 •

edited

Loading

pdecat commented Oct 7, 2021 •

edited

Loading