Support full resource names in autokey_config.folder

[tdbhacks]

Add pre_* custom code to drop the 'folders/' prefix from API requests, as well as a diff suppressor for the same reason. The ID also needs to be adjusted on post_create.

Side change: add a time delay after setting the autokey_config in the basic example, to try to address test flakiness (#18935)

NOTE: looking at the debug logs, I see the following error:

2024-08-09T16:37:55.977Z [WARN]  Provider "provider[\"registry.terraform.io/hashicorp/google-beta\"]" produced an unexpected new value for google_kms_autokey_config.example-autokeyconfig, but we are tolerating it because it is using the legacy plugin SDK.
    The following problems may be the cause of any confusing errors from downstream operations:
      - .folder: was cty.StringVal("folders/966201355962"), but now cty.StringVal("966201355962")

looking for guidance on whether my current approach is fine, or if this should be addressed in a different way.

Thank you!

Release Note Template for Downstream PRs (will be copied)

kms: updated the `google_kms_autokey_config` resource's `folder` field to accept values that are either full resource names (`folders/{folder_id}`) or just the folder id (`{folder_id}` only)

[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, 8 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 4 files changed, 38 insertions(+), 145 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 7 insertions(+))

[modular-magician]

Tests analytics

Total tests: 38
Passed tests: 34
Skipped tests: 4
Affected tests: 0

Click here to see the affected service packages

• kms

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

View the build log

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

[SarahFrench]

Can I just check, is this PR to help with a situation where a user might set the folder argument to folder/123456 and this results in the provider creating a malformed URL: https://cloudkms.googleapis.com/v1/folders/folder/123456/autokeyConfig ? Is there a related GitHub issue you could link to?

[tdbhacks]

Can I just check, is this PR to help with a situation where a user might set the folder argument to folder/123456 and this results in the provider creating a malformed URL: https://cloudkms.googleapis.com/v1/folders/folder/123456/autokeyConfig ? Is there a related GitHub issue you could link to?

You are spot on, this is to support the full resource name too in the folder argument, but avoid the duplicate folders/. No GitHub issue, but the internal chatter is at yaqs/2415509398479699968

[SarahFrench]

I'm afraid I'm a HashiCorp employee so I can't see Google internal docs.

Does the internal discussion mention making changes to validation on the field so that users are only able to pass in values without the folder/ prefix? This wouldn't break any existing valid configurations using the resource. However if values like folder/123456 are returned from other resouces/data sources without a field that contains only 123456 I can see how users might have a lot of toil navigating that validation. Let me know your thoughts!

[tdbhacks]

I'm afraid I'm a HashiCorp employee so I can't see Google internal docs.

Does the internal discussion mention making changes to validation on the field so that users are only able to pass in values without the folder/ prefix? This wouldn't break any existing valid configurations using the resource. However if values like folder/123456 are returned from other resouces/data sources without a field that contains only 123456 I can see how users might have a lot of toil navigating that validation. Let me know your thoughts!

ooops, apologies, my bad :)

As a summary:

• the folder argument only supports IDs right now (eg. 123456789), though it's more of an implicit symptom of how the URLs are defined, because..
• ..the URLs used in the .yaml resource definition expect such an ID, given that the folders/ prefix is hardcoded. See verbs definitions.
• some users have expressed to us that it would be good to support both IDs (123456789) and full resource names (folders/123456789) as valid inputs for the folder argument
• if I understand correctly, this wouldn't be a breaking change because ID-only values are still accepted after this change
• as part of accepting full resource names, we need to adjust the URLs used, to avoid the folders/folders/123456789 issue that you identified correctly
• my understanding of the diff suppressor is that it ensures 123456789 and folders/123456789 are considered the same for diff'ing purposes

Hope this helps, let me know if something doesn't make sense. @melinath and @zli82016 have chimed in on the internal discussions and can probably help here.

[SarahFrench]

Some quick comments, including a required code change to re-add a sweeper to the Beta version of the provider.

[SarahFrench]

Is it necessary to re-set (via d.Set) the value to be the original string with "folders/" removed? Is could be sufficient to just perform replacements of "folders/folders/" with "folders/" whenever a URL is being constructed using the folder value.

[tdbhacks]

Nope, you're absolutely right, this was leftover from my initial attempt :) removed

[SarahFrench]

The id issue raised below can be solved by this, like you said here. I can restore this on the PR for you

[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, 8 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 4 files changed, 32 insertions(+), 145 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 7 insertions(+))

[modular-magician]

Tests analytics

Total tests: 38
Passed tests: 33
Skipped tests: 4
Affected tests: 1

Click here to see the affected service packages

• kms

Action taken

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

• TestAccKMSAutokeyConfig_kmsAutokeyConfigAllExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccKMSAutokeyConfig_kmsAutokeyConfigAllExample[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

[tdbhacks]

@SarahFrench re: test failure, this is a known issue, and apparently my attempt at fixing this by adding a timeout is not sufficient / the right approach? :( let me know if you notice something else that might need to be tweaked. I can try adding an even longer wait time, though I thought 15s would be enough..

[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 ( 2 files changed, 134 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 4 files changed, 34 insertions(+), 21 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 7 insertions(+))

[modular-magician]

Tests analytics

Total tests: 38
Passed tests: 33
Skipped tests: 4
Affected tests: 1

Click here to see the affected service packages

• kms

Action taken

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

• TestAccKMSAutokeyConfig_kmsAutokeyConfigAllExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccKMSAutokeyConfig_kmsAutokeyConfigAllExample[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

[tdbhacks]

@SarahFrench alright, now that I've added back the sweeper, I've noticed that I'm probably handling the id incorrectly. What I'm doing right now is updating the id on post_create, which seemed like the right spot looking at where the code is injected, but I don't see that in the google-beta diffs, and if you look at the latest test failure, it's showing an id with the incorrect folders/folders/... value.

Thoughts?

[tdbhacks]

One more note: I have a feeling the leftover change I had before (ie. modifying the folder value on pre_create and pre_update) was taking care of this ID issue, but I agree that the behavior would feel weird

[SarahFrench]

Sorry, I've been pretty busy this past week! I'll review this PR on Monday

[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 ( 2 files changed, 134 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 4 files changed, 37 insertions(+), 21 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 7 insertions(+))

[modular-magician]

Tests analytics

Total tests: 40
Passed tests: 34
Skipped tests: 6
Affected tests: 0

Click here to see the affected service packages

• kms

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

View the build log

[tdbhacks]

@SarahFrench thank you! Anything else I should change or double-check to make sure everything works correctly?

[SarahFrench]

Nope, nothing more, I'll approve and merge. Thanks!