Make plugin-framework provider configuration code treat empty values like the SDK by SarahFrench · Pull Request #8798 · GoogleCloudPlatform/magic-modules

SarahFrench · 2023-08-30T00:06:36Z

This PR fixes hashicorp/terraform-provider-google#14255

This PR makes the plugin-framework code behave more like the SDK by ignoring empty strings, and is a bug fix to be included in a v4.x.x release. The bug it's fixing is an unintentional behaviour change introduced when the provider was muxed.

Later, I will remove this fix in 5.0.0, as an intentional breaking change as part of : hashicorp/terraform-provider-google#14447

Also, this PR changes some arbitrary strings from "123s" to "45s" as I realised that "123s" is changed to "2m3s" during the provider configuration logic. I thought this change would help avoid confusion if tests are updated in future to inspect where that second, parsed value is used.

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)

provider: fixed the provider so it resumes ignoring empty strings set in the `provider` block

modular-magician · 2023-08-30T00:16:56Z

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 ( 8 files changed, 195 insertions(+), 148 deletions(-))
Terraform Beta: Diff ( 8 files changed, 195 insertions(+), 148 deletions(-))

modular-magician · 2023-08-30T01:33:13Z

Tests analytics

Total tests: 3006
Passed tests 2710
Skipped tests: 296
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

SarahFrench · 2023-09-11T11:47:02Z

This PR currently contains all the unit tests I've managed to get approved and merged to main, unit tests I've got in open PRs, and code changes to make the PF config code behave more like the SDK config code w.r.t empty strings

modular-magician · 2023-09-11T11:53:07Z

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 ( 3 files changed, 503 insertions(+), 133 deletions(-))
Terraform Beta: Diff ( 3 files changed, 503 insertions(+), 133 deletions(-))

modular-magician · 2023-09-11T13:05:32Z

Tests analytics

Total tests: 3025
Passed tests 2728
Skipped tests: 297
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

…ng ENVs

SarahFrench

Added some comments to help with review

…to null values

…re overridden by an ENV

…are arbitary

This is because 123s is transformed to 2m3s, but 45s remains 45s

modular-magician · 2023-09-13T14:24:30Z

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 ( 3 files changed, 238 insertions(+), 147 deletions(-))
Terraform Beta: Diff ( 3 files changed, 238 insertions(+), 147 deletions(-))

modular-magician · 2023-09-13T15:36:42Z

Tests analytics

Total tests: 3037
Passed tests 2733
Skipped tests: 297
Affected tests: 7

Action taken

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

TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccGKEHub2Scope_gkehubScopeBasicExample|TestAccGKEHub2ScopeIamBindingGenerated|TestAccGKEHub2ScopeIamMemberGenerated|TestAccGKEHub2ScopeIamPolicyGenerated

Get to know how VCR tests work

modular-magician · 2023-09-13T15:50:12Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample[Debug log]
TestAccGKEHub2Scope_gkehubScopeBasicExample[Debug log]
TestAccGKEHub2ScopeIamBindingGenerated[Debug log]
TestAccGKEHub2ScopeIamMemberGenerated[Debug log]
TestAccGKEHub2ScopeIamPolicyGenerated[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\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 · 2023-09-13T17:45:41Z

+	// Make the plugin framwork code behave like the SDK by ignoring zero values. This means re-setting zero values to null.
+	// This is added to fix https://github.com/hashicorp/terraform-provider-google/issues/14255 in a v4.x.x release
+	// TODO(SarahFrench) remove as part of https://github.com/hashicorp/terraform-provider-google/issues/14447 in 5.0.0
+	p.HandleZeroValues(ctx, data, diags)
+	if diags.HasError() {
+		return
+	}


For 5.0.0, when we stop ignoring empty strings provided in a user's Terraform configuration, I plan to delete this section and remove the HandleZeroValues function

We are adding this functionality back for users that could not upgrade since 4.60 right? To give them access to remaining 4.x versions

Yeah that's right. Users affected by hashicorp/terraform-provider-google#14255 could navigate the issue easily by removing empty strings from their provider config block, but that becomes less easy if they have a lot of different provider configs or if an empty string was 'baked into' a module that a user didn't have control over.

By making this change we're allowing any users on old provider versions to upgrade to the final 4.x.x version prior to 5.0.0 with everything behaving as they'd expect. Then they'll have the upgrade guide warning them about empty strings no longer being ignored in 5.0.0.

c2thorn

Straightforward except for the batching bit, but looks correct.

SarahFrench · 2023-09-14T00:12:23Z

The batching bit was a pain for me to figure out, but it works! I'll do some final manual testing tomorrow and will merge this PR then. Thanks for the approval! 🚀

…ntials=""` and `GOOGLE_CREDENTIALS` interact

SarahFrench · 2023-09-14T16:28:18Z

Instead of manual testing, I've added an acceptance test in 16b27d4 that shows the behaviour (specifically for the bug mentioned in hashicorp/terraform-provider-google#14255) and how it's fixed following this PR. In future I'll update that test with another step when we intentionally reverse this bug fix in 5.0.0

modular-magician · 2023-09-14T16:34:47Z

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 ( 4 files changed, 307 insertions(+), 147 deletions(-))
Terraform Beta: Diff ( 4 files changed, 307 insertions(+), 147 deletions(-))

modular-magician · 2023-09-14T17:48:34Z

Tests analytics

Total tests: 3043
Passed tests 2745
Skipped tests: 298
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

SarahFrench · 2023-09-14T17:55:00Z

Following addition of that test and all checks passing, I'll merge this PR once merging to main is resumed

…like the SDK (GoogleCloudPlatform#8798) * Make PF config code change `project = ""` to null value before checking ENVs * Make plugin framework config code change all empty strings in config to null values * Fix defect in unit test for `project`, uncomment * Uncomment empty string unit tests * Fix defect in test; expeceted value should be null list * Add handling of empty lists in `HandleZeroValues` * Fix typo in comment * Add test case asserting `impersonate_service_account` empty strings are overridden by an ENV * Update SDK `batching` tests: rename test cases, make it clear values are arbitary * Update `HandleZeroValues` to handle `batching` argument * Uncomment empty string test * Change test inputs from 123s to 45s This is because 123s is transformed to 2m3s, but 45s remains 45s * Protect against Batching being null/unknown in `HandleZeroValues` * Add non-VCR acceptance test that shows provider behaviour when `credentials=""` and `GOOGLE_CREDENTIALS` interact

SarahFrench force-pushed the make-pf-treat-empty-values-null branch from 69d5653 to 2a7cffa Compare September 11, 2023 11:43

Make PF config code change project = "" to null value before checki…

ba851a7

…ng ENVs

SarahFrench force-pushed the make-pf-treat-empty-values-null branch 2 times, most recently from ea3c199 to 78cee10 Compare September 13, 2023 14:03

SarahFrench commented Sep 13, 2023

View reviewed changes

Comment thread mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb

Comment thread mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb

SarahFrench force-pushed the make-pf-treat-empty-values-null branch from 78cee10 to 53a3cf4 Compare September 13, 2023 14:07

SarahFrench added 12 commits September 13, 2023 15:12

Make plugin framework config code change all empty strings in config …

55e15c3

…to null values

Fix defect in unit test for project, uncomment

ab2cb71

Uncomment empty string unit tests

3b099a1

Fix defect in test; expeceted value should be null list

9add92e

Add handling of empty lists in HandleZeroValues

7013dac

Fix typo in comment

3a6f023

Add test case asserting impersonate_service_account empty strings a…

68900b3

…re overridden by an ENV

Update SDK batching tests: rename test cases, make it clear values …

b6bc2fd

…are arbitary

Update HandleZeroValues to handle batching argument

821a2aa

Uncomment empty string test

741127a

Change test inputs from 123s to 45s

7f43585

This is because 123s is transformed to 2m3s, but 45s remains 45s

Protect against Batching being null/unknown in HandleZeroValues

d5f4195

SarahFrench force-pushed the make-pf-treat-empty-values-null branch from 53a3cf4 to d5f4195 Compare September 13, 2023 14:13

SarahFrench marked this pull request as ready for review September 13, 2023 17:32

SarahFrench commented Sep 13, 2023

View reviewed changes

SarahFrench requested a review from c2thorn September 13, 2023 17:45

SarahFrench mentioned this pull request Sep 13, 2023

Fix provider configuration code to handle Unknown values correctly #8943

Merged

c2thorn approved these changes Sep 13, 2023

View reviewed changes

Add non-VCR acceptance test that shows provider behaviour when `crede…

16b27d4

…ntials=""` and `GOOGLE_CREDENTIALS` interact

SarahFrench merged commit aaccd4a into main Sep 14, 2023

This was referenced Sep 19, 2023

Fix provider configuration code to handle Unknown values correctly hashicorp/terraform-provider-google-beta#6312

Merged

Fix provider configuration code to handle Unknown values correctly hashicorp/terraform-provider-google#15898

Merged

SarahFrench mentioned this pull request Sep 21, 2023

Unignore empty values in the provider configuration block in 5.0.0 #9014

Merged

3 tasks

SarahFrench deleted the make-pf-treat-empty-values-null branch September 26, 2023 13:43

Uh oh!

Conversation

SarahFrench commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

modular-magician commented Aug 30, 2023

Diff report

Uh oh!

modular-magician commented Aug 30, 2023

Tests analytics

Uh oh!

SarahFrench commented Sep 11, 2023

Uh oh!

modular-magician commented Sep 11, 2023

Diff report

Uh oh!

modular-magician commented Sep 11, 2023

Tests analytics

Uh oh!

SarahFrench left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

modular-magician commented Sep 13, 2023

Diff report

Uh oh!

modular-magician commented Sep 13, 2023

Tests analytics

Action taken

Uh oh!

modular-magician commented Sep 13, 2023

Rerun these tests in REPLAYING mode to catch issues

Uh oh!

SarahFrench Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

c2thorn Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

SarahFrench Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

c2thorn left a comment

Choose a reason for hiding this comment

Uh oh!

SarahFrench commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SarahFrench commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

modular-magician commented Sep 14, 2023

Diff report

Uh oh!

modular-magician commented Sep 14, 2023

Tests analytics

Uh oh!

SarahFrench commented Sep 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SarahFrench commented Aug 30, 2023 •

edited

Loading

SarahFrench commented Sep 14, 2023 •

edited

Loading

SarahFrench commented Sep 14, 2023 •

edited

Loading