Test and handle time-based alert grouping parameters

[drastawi]

Setting time in the alert grouping parameters crashes the provider after #570 . This should be tested and fixed.
Marking as draft the API request does not appear to set the timeout as expected. (PD support investigating)

[drastawi]

@imjaroiswebdev the timeout still does not appear to be set correctly even after this addition. Likely there is a problem here on the API side that will get fixed eventually, but in the meantime, we should not allow the provider to crash. If we merge it as is we should be able to stop the crashes.

[pdecat]

This will probably resolve #579

[pdecat]

Hi @drastawi, if the behavior you are seing is related to setting zero values, it may be caused by the omitempty tags on the pagerduty.AlertGroupingConfig struct that drops those values during JSON marshalling: https://github.com/heimweh/go-pagerduty/blob/ceae303/pagerduty/service.go#L39-L44

[drastawi]

@pdecat the API request seems correct and gets a 200 response, but the change is not reflected, regardless if you set 0 or 5 as in my test. I tried submitting the same request via Postman directly and I see the same behavior. I tried escalating it to the premium support, but no response after a week.

[pdecat]

Indeed, nothing to do with zero values.

Here's a fix to make the test from your branch pass:

diff --git a/pagerduty/resource_pagerduty_service_test.go b/pagerduty/resource_pagerduty_service_test.go
index 7f086be5..29989ae8 100644
--- a/pagerduty/resource_pagerduty_service_test.go
+++ b/pagerduty/resource_pagerduty_service_test.go
@@ -283,7 +283,7 @@ func TestAccPagerDutyService_AlertContentGrouping(t *testing.T) {
                                        resource.TestCheckResourceAttr(
                                                "pagerduty_service.foo", "alert_grouping_parameters.0.type", "time"),
                                        resource.TestCheckResourceAttr(
-                                               "pagerduty_service.foo", "alert_grouping_parameters.0.config.0.timeout.0", "5"),
+                                               "pagerduty_service.foo", "alert_grouping_parameters.0.config.0.timeout", "5"),
                                        resource.TestCheckNoResourceAttr(
                                                "pagerduty_service.foo", "alert_grouping_parameters.0.config.0"),
                                        resource.TestCheckResourceAttr(

[pdecat]

Suggested change

	"pagerduty_service.foo", "alert_grouping_parameters.0.config.0.timeout.0", "5"),
	"pagerduty_service.foo", "alert_grouping_parameters.0.config.0.timeout", "5"),

[pdecat]

This function should be moved up to line 1087, right after testAccCheckPagerDutyServiceConfigWithAlertContentGroupingUpdated(), to keep the same order as for invocations in test steps.

https://github.com/PagerDuty/terraform-provider-pagerduty/pull/582/files#diff-afdb38b0f8715bc060e1cca7ee93367e1fd6e8f41ac67a59d9c7f7660e003041R1086

[imjaroiswebdev]

@drastawi the changes submitted with this patch looks pretty good, so I just pushed a commit for adding an extra test for the case when timeout is set to zero using a time based alert grouping configuration, more over I also add the suggestion from @pdecat in the tests, however by my side I haven't been able to replicate the issue with the update of the Alert Grouping Parameters config, I've running some tests to validate this behaviour, getting the expected result though. So please @drastawi could you provide a more of context or confirm you are still facing this issue? because in regards of this patch it's looks complete and doing the correct API calls.

[drastawi]

Hey @imjaroiswebdev I see the API is indeed working correctly with the config block now on my private pagerduty.com instance, but it is still not working on our company's PagerDuty instance. Seems like they may be in the process of releasing a fix now. Since tests work, then let's go ahead and merge it in as my issues have nothing to do with terraform.

Small note on the terraform side, I still see the previous "alert_grouping" parameter included in the API request from the terraform state even though it does not exist in the config. So for example when we call alert_grouping.parameters.config[0].type.timeout, the alert_grouping.rules is also sent as It was set in the previous test and remained in the state. It does not seem to break anything in any of the tests though so maybe not worth fixing here.

[pdecat]

Small note on the terraform side, I still see the previous "alert_grouping" parameter included in the API request from the terraform state even though it does not exist in the config. So for example when we call alert_grouping.parameters.config[0].type.timeout, the alert_grouping.rules is also sent as It was set in the previous test and remained in the state. It does not seem to break anything in any of the tests though so maybe not worth fixing here.

That's probably caused by the omitempty tags.

[imjaroiswebdev]

Small note on the terraform side, I still see the previous "alert_grouping" parameter included in the API request from the terraform state even though it does not exist in the config. So for example when we call alert_grouping.parameters.config[0].type.timeout, the alert_grouping.rules is also sent as It was set in the previous test and remained in the state. It does not seem to break anything in any of the tests though so maybe not worth fixing here.

@drastawi I agree with you that this update looks to be ready to be merged, about what you notice, I'll do more tests and also I'm going to validate if the omitempty might be the solution as @pdecat pointed out, so I would be letting you know in a new PR, but back to this PR this is ready to be merged and release today. Guys thank you so much for your help and contribution. 💪🏽