cloud_tasks_queue: supress permadiffs on backoff settings

[wyardley]

Add tpgresource.DurationDiffSuppress to min / max backoff
Fixes hashicorp/terraform-provider-google#15166

h/t to @edwardmedia for the pointer

Note: maxRetryDuration already has a custom diff function: suppressOmittedMaxDuration() - for now, I didn't touch that one

If this PR is for Terraform, I acknowledge that I have:

• Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
• Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
• Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
• Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
• Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

cloud_tasks: suppressed time-unit permadiffs on `google_cloud_tasks_queue` min and max backoff settings

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @slevenick, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

---

Your assigned reviewer will help review your code by:

• Ensuring it's backwards compatible, covers common error cases, etc.
• Summarizing the change into a user-facing changelog note.
• Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

---

[wyardley]

So far, adding diff_suppress_func doesn't seem to modify the resource code when I generate providers; not sure if that's expected. Working on getting setup to run lint / unit tests locally again, but added a simple copy / paste test that I think will test for the issue?

[slevenick]

So far, adding diff_suppress_func doesn't seem to modify the resource code when I generate providers; not sure if that's expected. Working on getting setup to run lint / unit tests locally again, but added a simple copy / paste test that I think will test for the issue?

It should cause a change in the provider, I'd guess you don't have the local environment set up correctly. I've triggered the run so it should show the diff here in ~15 minutes

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 48 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 2 files changed, 48 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 13
Passed tests 13
Skipped tests: 0
Affected tests: 0

$\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR}}$
View the build log

[slevenick]

Your test config function doesn't match up to what is being called.

testAccCloudtasksQueueTimeUnitDiff vs testAccCloudtasksQueueRetryTimeUnitDiff

[wyardley]

Your test config function doesn't match up to what is being called.

Derp, yeah, sorry, updated in one place but not the other. updated.

Thoughts on whether we need to solve this in maxRetryDuration too? I assume it's not trivial to add two functions, but also seems janky to copy / paste into the existing custom function?

[slevenick]

Your test config function doesn't match up to what is being called.

Derp, yeah, sorry, updated in one place but not the other. updated.

Thoughts on whether we need to solve this in maxRetryDuration too? I assume it's not trivial to add two functions, but also seems janky to copy / paste into the existing custom function?

Judging by the name that looks to solve a different problem. Do you see this same problem on maxRetryDuration?

The existing code in that DSF just equalizes "0" and ""

[wyardley]

I'd guess you don't have the local environment set up correctly.

Yeah, possible.

I was generating some diffs, just not the expected ones. (the test diffs got created as expected, though)

I have successfully done this before, and have run through all the setup docs in the past, and reviewed them again. The doctor script didn't report any problems. Still dealing with some go dependency issues for running vet / lint / etc., but this should be happening before that, I think. Anyway, I'll get it working again, esp. when I next dig into anything trickier than this.

[wyardley]

Judging by the name that looks to solve a different problem. Do you see this same problem on maxRetryDuration?

That's right - the existing custom code (and test) seems to be handling the situation where it's 0. Since it accepts time units, I guess I was expecting that the same issue would likely affect maxRetryDuration -- it hasn't come up for me specifically, but it would seem like it could?

I am not suggesting that we remove the existing custom function for it, just that we might also ideally want to incorporate the behavior of tpgresource.DurationDiffSuppress as well, if you follow.

[slevenick]

Judging by the name that looks to solve a different problem. Do you see this same problem on maxRetryDuration?

That's right - the existing custom code (and test) seems to be handling the situation where it's 0. Since it accepts time units, I guess I was expecting that the same issue would likely affect maxRetryDuration -- it hasn't come up for me specifically, but it would seem like it could?

I am not suggesting that we remove the existing custom function for it, just that we might also ideally want to incorporate the behavior of tpgresource.DurationDiffSuppress as well, if you follow.

Yeah, we could incorporate the behavior in there by calling the duration DSF. I'd make sure we actually see the problem before implementing that fix though

[wyardley]

@slevenick

Yes, as you'd guess, the same behavior affects max_retry_duration - see the example below

resource "google_cloud_tasks_queue" "foobarbaz" {
  name     = "foobarbaz"
  location = "us-west2"
  rate_limits {
    max_dispatches_per_second = 8
    max_concurrent_dispatches = 2
  }   
  retry_config {
    max_attempts = 2
    max_backoff = "2.5s"
    min_backoff = "2.000s"
    max_doublings = 5
    max_retry_duration = "10.0s"
  }
}    

will result in this permadiff

      ~ retry_config {
          ~ max_backoff        = "2.500s" -> "2.5s"
          ~ max_retry_duration = "10s" -> "10.0s"
          ~ min_backoff        = "2s" -> "2.000s"

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 48 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 2 files changed, 48 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2855
Passed tests 2555
Skipped tests: 297
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules|TestAccCloudTasksQueue_TimeUnitDiff

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccCloudTasksQueue_TimeUnitDiff[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

[wyardley]

The test as written will already check that there's no drift after apply, right? Or do I have to do the thing where it does it a second time?

Yeah, we could incorporate the behavior in there by calling the duration DSF

I can add max_retry_duration to the existing test (which I think should then fail); do you have an example or suggestion of how I'd do that? Just call tpgresource.DurationDiffSuppress() within suppressSuffixDiff()?

[slevenick]

The test as written will already check that there's no drift after apply, right? Or do I have to do the thing where it does it a second time?

Yeah, we could incorporate the behavior in there by calling the duration DSF

I can add max_retry_duration to the existing test (which I think should then fail); do you have an example or suggestion of how I'd do that? Just call tpgresource.DurationDiffSuppress() within suppressSuffixDiff()?

The test should check there's no diff after apply. You can test that out by changing the max_retry_duration to a value that causes the issue.

I think you should be able to call the DurationDiffSuppress from within the existing handwritten diff suppress that field is using, just by replacing:

return false

with

return tpgresource.DurationDiffSuppress(ARGS HERE)

[wyardley]

Sounds good. I can't run the integration tests locally, so I'll push up the change with the presumably breaking test.

Assuming that looks good (bad), I'll push up the change with your suggestion for calling the additional suppression function; thanks for the example.

[wyardley]

So like this?

diff --git a/mmv1/templates/terraform/constants/cloud_tasks_retry_config_custom_diff.go b/mmv1/templates/terraform/constants/cloud_tasks_retry_config_custom_diff.go
index cba219445..9f65d60e8 100644
--- a/mmv1/templates/terraform/constants/cloud_tasks_retry_config_custom_diff.go
+++ b/mmv1/templates/terraform/constants/cloud_tasks_retry_config_custom_diff.go
@@ -1,7 +1,11 @@
+import (
+        "github.com/GoogleCloudPlatform/terraform-google-conversion/v2/tfplan2cai/converters/google/resources/tpgresource"
+)
+
 func suppressOmittedMaxDuration(_, old, new string, _ *schema.ResourceData) bool {
 	if old == "" && new == "0s" {
 		log.Printf("[INFO] max retry is 0s and api omitted field, suppressing diff")
 		return true
 	}
-	return false
+	return tpgresource.DurationDiffSuppress(old, new)
 }

[wyardley]

@slevenick ok, I had to pass the other two parameters too, but seems to work (and confirmed that the import is already there / not needed)! 🎉
I just used k and d for the var names, let me know if I need to use something more descriptive.

I fixed the local build and confirms that this works locally to resolve the issue.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 50 insertions(+), 8 deletions(-))
Terraform Beta: Diff ( 2 files changed, 50 insertions(+), 8 deletions(-))
TF Conversion: Diff ( 3 files changed, 5 insertions(+), 5 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2864
Passed tests 2562
Skipped tests: 299
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccCloudTasksQueue_TimeUnitDiff|TestAccComputeFirewallPolicyRule_multipleRules|TestAccContainerAwsNodePool_BetaBasicHandWritten

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccCloudTasksQueue_TimeUnitDiff[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 50 insertions(+), 8 deletions(-))
Terraform Beta: Diff ( 2 files changed, 50 insertions(+), 8 deletions(-))
TF Conversion: Diff ( 3 files changed, 5 insertions(+), 5 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2872
Passed tests 2571
Skipped tests: 299
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

[wyardley]

@slevenick let me know how this looks now
I think the failing tests are not related, right?

I can rebase the branch if you'd like, though sometimes that slows things down, so I will leave it as-is unless you want me to.