Add diff suppress for time format to google_compute_resource_policies

[karolgorc]

closes issue #8460

This is probably a breaking change because some terraform apply commands won't go through if they have a wrong format. The regex will throw an error. But it also ensures that it's the correct format for the API.

Will write a test case for this into the PR if it's an approved solution.

Release Note Template for Downstream PRs (will be copied)

compute: fixed a permadiff caused by setting `start_time` in an incorrect H:mm format in `google_compute_resource_policies` resources

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@SarahFrench, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[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.

google provider: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))

[modular-magician]

Tests analytics

Total tests: 982
Passed tests: 908
Skipped tests: 72
Affected tests: 2

Click here to see the affected service packages

• compute

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

• TestAccComputeDisk_storagePoolSpecified
• TestAccComputeInstance_bootDisk_storagePoolSpecified

Get to know how VCR tests work

[modular-magician]

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

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

[SarahFrench]

Hi, thanks for your PR!

My understanding of the problem is that currently if a user was to use "1:00" in their configuration Terraform will identify a permadiff because the API will return "01:00" and there's a mismatch as a result.

The new regex for validating input looks correct to me, but you're also correct that this would be a breaking change for some users - e.g. some might have used ignore_changes on this field to navigate the permadiff.

Another approach is to add diff suppression to the field so that the provider regards "1:00" and "01:00" (for example) as equivalent and doesn't raise a diff in this situation. Would you consider this alternative instead, as it's non-breaking?

[karolgorc]

ok i wasn't aware that the Google API actually translated the given value from H:MM to HH:MM. If that happens then the suppress diff seems like a better solution

• Added a HourlyFormatSuppressDiff() to start_time under hourly_schedule and daily_schedule
• Added unit test for it

[github-actions]

@SarahFrench This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[SarahFrench]

@SarahFrench This PR has been waiting for review for 3 weekdays. Please take a look!

I'm OOO and will review this on Tuesday

[SarahFrench]

This looks good to me, thanks for the testing of the diff suppression function! Before merging I'd like there to be an acceptance test that contains a start_time value in H:mm format and shows that there aren't errors.

An approach for this could be to update this example file, which is referenced here in ResourcePolicy.yaml , so that the start_time value can be passed in from the YAML as a variable (similar to what you see for name). After doing this, you could add another entry to the examples list in ResourcePolicy.yaml but set the value for start_time in the H:mm format. You'd also want to include skip_docs: true in that block, so that the example is only used to generate a test and doesn't accidentally promote using H:mm format format in the official provider documentation

[karolgorc]

Using the same example file generates 2 test functions that have the same name. If there is a way to generate 2 unique testing functions using the same example file please LMK! But for now i just created another file that stores the new config with 4:00 instead of 04:00.

--- PASS: TestAccComputeResourcePolicy_resourcePolicyHourlyFormatExample (12.39s)
PASS
ok      github.com/hashicorp/terraform-provider-google/google/services/compute    12.417s

[SarahFrench]

Ah, of course name collision would happen 🤦 . In that case I'm afraid that the route forward is to create a new example file with a different name that has the start_time value set in H:mm. Because of this having the time set via a variable wouldn't be necessary. Sorry for leading you down an incorrect path!

[karolgorc]

In that case I'm afraid that the route forward is to create a new example file with a different name that has the start_time value set in H:mm

Done here

[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.

google provider: Diff ( 3 files changed, 97 insertions(+), 6 deletions(-))
google-beta provider: Diff ( 3 files changed, 97 insertions(+), 6 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 6 insertions(+))
Open in Cloud Shell: Diff ( 4 files changed, 113 insertions(+))

[modular-magician]

Tests analytics

Total tests: 990
Passed tests: 911
Skipped tests: 77
Affected tests: 2

Click here to see the affected service packages

• compute

#### Non-exercised tests

Tests were added that are skipped in VCR:

• TestHourlyFormatSuppressDiff

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

• TestAccComputeInstanceTemplate_withNamePrefix
• TestAccComputeResourcePolicy_resourcePolicyHourlyFormatExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeInstanceTemplate_withNamePrefix[Debug log]
TestAccComputeResourcePolicy_resourcePolicyHourlyFormatExample[Debug log]
$\textcolor{red}{\textsf{Tests failed when rerunning REPLAYING mode:}}$
TestAccComputeInstanceTemplate_withNamePrefix[Error message] [Debug log]

Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made.

Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.

---

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

[SarahFrench]

Thanks, and sorry again! The new generated test passes so I'll merge. The TestAccComputeInstanceTemplate_withNamePrefix failure is unrelated to this PR.