Skip to content
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(multilocation_alert_condition): make violation_time_limit_seconds nearly consistent with its usage in nrql_alert_condition #2493

Merged

Conversation

pranav-new-relic
Copy link
Member

@pranav-new-relic pranav-new-relic commented Oct 23, 2023

As reported in #2475, the current validation against the argument violation_time_limit_seconds does not allow using the value 259200 (30 days), while this is supposed to be allowed as per New Relic's docs.

This attribute violation_time_limit_seconds is also used in newrelic_nrql_alert_condition to serve the same usecase, but it appears that the validation and other schema specifications are not the same in both of these resources. This PR aims at correcting the above.

Additionally, since 0 has been permitted to be valid value of the violation_time_limit_seconds argument so far (though values of violation_time_limit_seconds are expected to ∈ [300, 2592000] like specified with newrelic_nrql_alert_condition), a DiffSuppressFunc has been added to violation_time_limit_seconds in the multilocation_alert_condition resource to handle the case in which users specify 0, but the API defaults 0 to 259200 (3 days), and hence, differences need to be suppressed.

… consistent with its usage in nrql_alert_condition
@pranav-new-relic pranav-new-relic force-pushed the fix/violation_time_limit_multilocation_alert_condition branch from a8a493d to febc019 Compare October 23, 2023 09:24
@codecov-commenter
Copy link

Codecov Report

Merging #2493 (febc019) into main (cbed7ad) will decrease coverage by 0.03%.
The diff coverage is 36.36%.

@@            Coverage Diff             @@
##             main    #2493      +/-   ##
==========================================
- Coverage   33.90%   33.87%   -0.03%     
==========================================
  Files          91       91              
  Lines       24900    24900              
==========================================
- Hits         8442     8435       -7     
- Misses      16303    16309       +6     
- Partials      155      156       +1     
Files Coverage Δ
newrelic/resource_helpers.go 1.38% <ø> (ø)
newrelic/resource_newrelic_nrql_alert_condition.go 30.66% <ø> (ø)
...wrelic_synthetics_multilocation_alert_condition.go 43.82% <36.36%> (-4.33%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ValidateFunc: validation.IntInSlice([]int{0, 3600, 7200, 14400, 28800, 43200, 86400}),
Description: "The maximum number of seconds an incident can remain open before being closed by the system. Must be one of: 0, 3600, 7200, 14400, 28800, 43200, 86400",
Optional: true,
ValidateFunc: validation.IntBetween(0, violationTimeLimitSecondsMax),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to be 300 - 2592000 as also specified in New Relic docs, but I've added 0 as the minimum as this resource is already in use with 0 as a valid value of violation_time_limit_seconds.

However, the current behaviour of the API is such that if 0 is given as a value against this argument, the API defaults it to 259200 seconds. This is why the below DiffSuppressFunc has been added.

@pranav-new-relic pranav-new-relic marked this pull request as ready for review October 23, 2023 11:57
@pranav-new-relic pranav-new-relic merged commit 652d27c into main Oct 23, 2023
6 checks passed
@pranav-new-relic pranav-new-relic deleted the fix/violation_time_limit_multilocation_alert_condition branch October 23, 2023 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants