-
Notifications
You must be signed in to change notification settings - Fork 212
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
Test and handle time-based alert grouping parameters #582
Conversation
dd0472e
to
30d1c33
Compare
@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. |
This will probably resolve #579 |
Hi @drastawi, if the behavior you are seing is related to setting zero values, it may be caused by the |
@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. |
Indeed, nothing to do with zero values. Here's a fix to make the test from your branch pass:
|
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"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"pagerduty_service.foo", "alert_grouping_parameters.0.config.0.timeout.0", "5"), | |
"pagerduty_service.foo", "alert_grouping_parameters.0.config.0.timeout", "5"), |
@@ -1093,6 +1121,47 @@ resource "pagerduty_service" "foo" { | |||
`, username, email, escalationPolicy, service) | |||
} | |||
|
|||
func testAccCheckPagerDutyServiceConfigWithAlertTimeGroupingUpdated(username, email, escalationPolicy, service string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should be moved up to line 1087, right after testAccCheckPagerDutyServiceConfigWithAlertContentGroupingUpdated()
, to keep the same order as for invocations in test steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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. |
That's probably caused by the |
@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 |
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)