-
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
Fix alert_grouping and alert_grouping_timeout conflicting with alert_grouping_parameters #377
Conversation
}, | ||
"alert_grouping_timeout": { | ||
Type: schema.TypeInt, | ||
Optional: true, | ||
Type: schema.TypeString, |
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.
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
} | ||
// Using GetOkExists to allow for alert_grouping_timeout to be set to 0 if needed. | ||
if attr, ok := d.GetOkExists("alert_grouping_timeout"); ok { |
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.
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.
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
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.
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.
b42cdfa
to
ee8f976
Compare
Rebased on master, @stmcallister PTAL. |
ee8f976
to
400e808
Compare
@pdecat Amazing work on this and the other related PRs! Thank you so much! On this one I'm still getting the following error
|
Weird, it is working fine for me:
PS: it would awesome to get acceptance tests running in CI |
…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
400e808
to
e3d3b7f
Compare
Rebased on master. |
|
@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
|
This can be checked with the GET abilities API endpoint which returns 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. |
@pdecat I do have |
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.
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.
This PR resolves issues with
alert_grouping
andalert_grouping_timeout
which are mostly visible since the introduction ofalert_grouping_parameters
in #342.Indeed
alert_grouping
andalert_grouping_timeout
are conflicting withalert_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:Note how the update of
alert_grouping
tointelligent
is ignored by the API.Example update API call when running
TestAccPagerDutyService_AlertGrouping
with changes from this PR:Note:
alert_grouping
andalert_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 withalert_grouping_parameters
which is especially important as the latter takes precedence over them.