Terraform-provider-newrelic: Inconsistency around guidance for violation_time_limit

[Kiran01bm]

This is related to violation_time_limit.

Documentation Says

https://registry.terraform.io/providers/newrelic/newrelic/latest/docs/resources/nrql_alert_condition#violation_time_limit

violation_time_limit - (Optional*) DEPRECATED: Use violation_time_limit_seconds instead.

Execution logs suggest the reverse

Warning: "violation_time_limit_seconds": [DEPRECATED] use `violation_time_limit` attribute instead

[jeffalder]

In addition, GraphQL marks violationTimeLimit deprecated: "Replaced by violationTimeLimitSeconds".

Also, if you specify violation_time_limit_seconds, terraform plan sees it as an added attribute; it doesn't correctly compare between the two.

At the very least, I think we need to reverse the execution log deprecation notice (I suspect this will fix the same issue I see in IntelliJ IDEA). But we also need additional code in order to recognize that violation_time_limit: TWENTY_FOUR_HOURS is the same as violation_time_limit_seconds: 86400, and it doesn't need to be updated right away.

[jeffalder]

Seems related to #1015

[zlesnr]

I believe violation_time_limit_seconds is the new correct field, which we originally deprecated because the API changed between RESTv2 and GraphQL, and now it has changed again. I'd probably suggest we keep only one of the attributes instead of trying to map one to the other. I'm not sure what that means for semver since we're reversing the decision to deprecate the violation_time_limit_seconds in the first place.

The team probably won't be looking at terraform much this month. PRs welcome as always. I just wanted to note what I remember from this.

[jeffalder]

IMO, reversing a deprecation should not trigger a major semver. It worked before and it works now. Removal of a deprecated field should trigger a major, obviously. I think that's OK as long as the field was deprecated for multiple minors prior to that. It's handwavey, but that follows from semver itself.

I might poke at that a bit later, but I'm _not_ a Go developer, and getting started on Terraform was a bit of a challenge, so I'm not sure how good it'll be.

[jaymcgrath]

I can poke at this next week as well. I did the original deprecation swap work and missed swapping out the [DEPRECATED] tag in the log output. I'm surprised to see both fields able to be set, there might be an issue with the way I set the fields as conflictsWith in the schema

@Kiran01bm - more info about the reasoning behind swapping the deprecation of the two fields can be found in this post to the NR Explorers Hub

[jthurman42]

I can not reproduce this with v2.20.0