Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

SarahFrench
Copy link
Collaborator

@SarahFrench SarahFrench commented Sep 20, 2023

First PR to address hashicorp/terraform-provider-google#14447

This PR stops the provider ignoring empty strings in provider blocks; if the value is in a user's config then it should be used by the code or removed by the user.

After muxing the provider we accidentally changed its behaviour to stop ignoring empty strings. That behaviour is what we'd expect the correct behaviour to be, so we are reverting to the old behaviour in a 4.x.x release (see #8798) and then intentionally making the switch to the new behaviour as part of 5.0.0

ALSO I've decided against making similar changes in #9015 because it would result in breaking changes for users when any datasources/resources are migrated to the plugin framework.

To do:

  • Remove validation of empty strings
  • Remove code that converts empty values to null values (awaiting sync from main to release branch)
  • Update tests

Release Note Template for Downstream PRs (will be copied)

provider: Empty strings in the provider configuration block will no longer be ignored when configuring the provider

@google-cla

This comment was marked as resolved.

@SarahFrench SarahFrench changed the base branch from main to FEATURE-BRANCH-major-release-5.0.0 September 20, 2023 10:54
@SarahFrench
Copy link
Collaborator Author

/gcbrun

@modular-magician
Copy link
Collaborator

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 ( 1 file changed, 1 insertion(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 1 insertion(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3035
Passed tests 2730
Skipped tests: 304
Affected tests: 1

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
TestAccVertexAIIndexEndpoint_updated

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

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

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 ( 1 file changed, 1 insertion(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 1 insertion(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3068
Passed tests 2755
Skipped tests: 307
Affected tests: 6

Action taken

Found 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccBigQueryDataTable_bigtable|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccDataprocJobIamPolicy|TestAccSpannerDatabaseIamPolicy|TestAccVertexAIIndexEndpoint_updated

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryDataTable_bigtable[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample[Debug log]
TestAccDataprocJobIamPolicy[Debug log]
TestAccSpannerDatabaseIamPolicy[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:}}$
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange[Error message] [Debug log]
TestAccVertexAIIndexEndpoint_updated[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

@SarahFrench SarahFrench changed the title Unignore empty vales in 5.0.0 Unignore empty vales in the provider configuration block in 5.0.0 Sep 21, 2023
@SarahFrench SarahFrench marked this pull request as ready for review September 21, 2023 15:28
@modular-magician
Copy link
Collaborator

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

@rileykarson
Copy link
Member

Looks like there's a unit test failure!

Also, is it surprising that we generated a TC config in TPG but not TPGB?

@SarahFrench
Copy link
Collaborator Author

SarahFrench commented Sep 21, 2023

Looks like there's a unit test failure!

Ah, thanks for pointing it out!

Also, is it surprising that we generated a TC config in TPG but not TPGB?

Yeah, very surprising 🤔 It's a sign that the packages.kt file isn't being generated properly when the downstream is made- I'll look into it

Edit: I can't see any evidence of that generated/packages.kt file being changed unexpectedly in the TPG/TPGB repos, so I think think there's a real problem. That file is generated by iterating through package folders, so it could be that there's a bit of a race condition?

Edit 2: this might be something that's specific to contributing to the 5.0.0 release - I see the same here #8587 (comment)

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3068
Passed tests 2758
Skipped tests: 307
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
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccSpannerDatabaseIamPolicy|TestAccVertexAIIndexEndpoint_updated

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccSpannerDatabaseIamPolicy[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:}}$
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange[Error message] [Debug log]
TestAccVertexAIIndexEndpoint_updated[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
Copy link
Collaborator

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 ( 6 files changed, 60 insertions(+), 140 deletions(-))
Terraform Beta: Diff ( 6 files changed, 60 insertions(+), 140 deletions(-))

@SarahFrench SarahFrench changed the title Unignore empty vales in the provider configuration block in 5.0.0 Unignore empty values in the provider configuration block in 5.0.0 Sep 21, 2023
@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3068
Passed tests 2757
Skipped tests: 307
Affected tests: 4

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccVertexAIIndexEndpoint_updated

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample[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:}}$
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange[Error message] [Debug log]
TestAccVertexAIIndexEndpoint_updated[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

@SarahFrench SarahFrench merged commit 83df041 into FEATURE-BRANCH-major-release-5.0.0 Sep 22, 2023
12 of 13 checks passed
@SarahFrench
Copy link
Collaborator Author

Ack, I forgot the guide - I'll make the PR for that now

SarahFrench added a commit that referenced this pull request Sep 22, 2023
@SarahFrench SarahFrench deleted the unignore-empty-vales-in-5.0.0 branch September 26, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants