google_storage_bucket: fix custom_placement_config values not normalized

[BBBmau]

Resolves hashicorp/terraform-provider-google#18035

Solution was to use StateFunc which converts the values in data_locations to be set as uppercase before storing into state. However as stated in the linked issue, there's a known problem with SDK that when using StateFunc in a nested attribute such as type.Set it will add the new value instead of replacing.

But by ignoring values in data_locations when expanding that aren't all capitalized, we can ensure that the final state will only include what we specified in StateFunc

for _, raw := range l {
		if raw.(string) == strings.ToUpper(raw.(string)) {
			req = append(req, raw.(string))
		}
	}
}

test output:
TestAccStorageBucket_dualLocation_lowercase - newly added test

(base) ┌─(~/Dev/terraform-provider-google)───────────────────────────────────────(mau@mau-JKDT676NCP:s008)─┐
└─(14:40:36 on main ✹)──> envchain GCLOUD make testacc TEST=./google/services/storage TESTARGS='-run=TestAccStorageBucket_dualLocation_lowercase'                                                    ──(Mon,Jun10)─┘
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/storage -v -run=TestAccStorageBucket_dualLocation_lowercase -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccStorageBucket_dualLocation_lowercase
=== PAUSE TestAccStorageBucket_dualLocation_lowercase
=== CONT  TestAccStorageBucket_dualLocation_lowercase
--- PASS: TestAccStorageBucket_dualLocation_lowercase (20.90s)
PASS
ok      github.com/hashicorp/terraform-provider-google/google/services/storage  22.114s

TestAccStorageBucket_dualLocation_versionChange - newly added test to ensure that behavior for user doesn't change:

(base) ┌─(~/Dev/terraform-provider-google)───────────────────────────────────────(mau@mau-JKDT676NCP:s008)─┐
└─(12:10:48 on main ✹)──> envchain GCLOUD make testacc TEST=./google/services/storage TESTARGS='-run=TestAccStorageBucket_dualLocation_versionChange'
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/storage -v -run=TestAccStorageBucket_dualLocation_versionChange -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccStorageBucket_dualLocation_versionChange
--- PASS: TestAccStorageBucket_dualLocation_versionChange (48.35s)
PASS

TestAccStorageBucket_dualLocation:

(base) ┌─(~/Dev/terraform-provider-google)──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s008)─┐
└─(14:48:58 on main ✹)──> envchain GCLOUD make testacc TEST=./google/services/storage TESTARGS='-run=TestAccStorageBucket_dualLocation'                                                              ──(Mon,Jun10)─┘
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/storage -v -run=TestAccStorageBucket_dualLocation -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccStorageBucket_dualLocation
=== PAUSE TestAccStorageBucket_dualLocation
=== RUN   TestAccStorageBucket_dualLocation_lowercase
=== PAUSE TestAccStorageBucket_dualLocation_lowercase
=== RUN   TestAccStorageBucket_dualLocation_rpo
=== PAUSE TestAccStorageBucket_dualLocation_rpo
=== CONT  TestAccStorageBucket_dualLocation
=== CONT  TestAccStorageBucket_dualLocation_rpo
=== CONT  TestAccStorageBucket_dualLocation_lowercase
--- PASS: TestAccStorageBucket_dualLocation_lowercase (21.61s)
--- PASS: TestAccStorageBucket_dualLocation (23.27s)
--- PASS: TestAccStorageBucket_dualLocation_rpo (49.14s)
PASS
ok      github.com/hashicorp/terraform-provider-google/google/services/storage  50.278s

Release Note Template for Downstream PRs (will be copied)

storage: fixed `custom_placement_config` values not normalized in `google_storage_bucket`

[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, 42 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 42 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 95
Passed tests: 88
Skipped tests: 6
Affected tests: 1

Click here to see the affected service packages

• storage

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

TestAccStorageBucket_dualLocation_lowercase

Get to know how VCR tests work

[modular-magician]

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

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

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[SarahFrench]

Could you avoid StateFunc and its limitations by making the flattener function downcase all strings before they're stored in state?

[BBBmau]

Could you avoid StateFunc and its limitations by making the flattener function downcase all strings before they're stored in state?

this would work for when the user inputs lowercase values but if users set as all caps they'll get diffs since state is set to ASIA_SOUTHEAST1 by user but downcasing the flattener returns us asia_southeast1

[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, 118 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 118 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

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

Click here to see the affected service packages

• storage

Non-exercised tests

Tests were added that are skipped in VCR:

• TestAccStorageBucket_dualLocation_lowercase
• TestAccStorageBucket_dualLocation_lowercase
• TestAccStorageBucket_dualLocation_versionChange

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

[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, 95 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 95 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 96
Passed tests: 89
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

• storage

Non-exercised tests

Tests were added that are skipped in VCR:

• TestAccStorageBucket_dualLocation_versionChange

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

[BBBmau]

@roaks3 hi! let me know if there's anything else needed for this review 👍🏼

[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, 94 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 94 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 96
Passed tests: 89
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

• storage

Non-exercised tests

Tests were added that are skipped in VCR:

• TestAccStorageBucket_dualLocation_versionChange

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

[github-actions]

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

[roaks3]

For the most part, this seems reasonable, and looks like it's working to me. But it is a bit unusual, so I'd like to understand the solution a bit better if possible:

1. I'm not quite understanding the trick to filter for uppercased values in the expander. Could you explain how it is working? It seems to handle the migration between provider versions well, but are there other types of changes to the config that might cause an issue?
2. Probably naive, but I can't help but wonder why a diff suppress doesn't work instead. It seems like the API accepts lower or uppercase, and returns uppercase?

[BBBmau]

@roaks3

1. I'm not quite understanding the trick to filter for uppercased values in the expander. Could you explain how it is working? It seems to handle the migration between provider versions well, but are there other types of changes to the config that might cause an issue?

Normally StateFunc would just replace the value but when using it with lists and sets it appends to it rather than replacing, causing data_locations = ["asia-east1", "asia-southeast1"] to be ["asia-east1", "asia-southeast1", "ASIA-EAST1", "ASIA-SOUTHEAST1"] after StateFunc

The added logic filters the list that StateFunc produces in expand to only keep Upper strings. we can do this by comparing a before and after of applying .ToUpper on each string.

func expandBucketDataLocations(configured interface{}) []string {
	l := configured.(*schema.Set).List()

	req := make([]string, 0, len(l))
	for _, raw := range l {
		if raw.(string) == strings.ToUpper(raw.(string)) {
			req = append(req, raw.(string))
		}
	}
	return req
}

For the most part I wasn't able to come up with other configs that could potentially cause issues with this. It handles lowercase and uppercase list configs well. Let me know if there's still something in my explanation that wasn't clear with this approach. I agree that the solution is unusual and comes from needing to workaround the limitations of StateFunc and DiffSuppressFunc with type.Set.

2. Probably naive, but I can't help but wonder why a diff suppress doesn't work instead. It seems like the API accepts lower or uppercase, and returns uppercase?

DiffSuppressFunc isn't supported on types Maps,Sets, and Lists, I did see that a workaround exists for map that was used here: #9452

Look into implementing a workaround for using it on Sets but not coming across a solution yet, I can spend more time it next week but that's the main reason why DiffSuppressFunc isn't being implemented, related discussion on DiffSuppresFunc on sets: hashicorp/terraform-plugin-sdk#477

[roaks3]

Ah ok, thanks for explaining, that all makes sense. I personally don't think it requires more investigation into using DiffSuppressFunc, but it might be worth adding a comment or two so the reader of the code knows why this works.

[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, 98 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 98 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 96
Passed tests: 89
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

• storage

Non-exercised tests

Tests were added that are skipped in VCR:

• TestAccStorageBucket_dualLocation_versionChange

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log