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

Add validation to provider fields in both SDK and PF implementations of provider schema #9050

Merged

Conversation

SarahFrench
Copy link
Collaborator

@SarahFrench SarahFrench commented Sep 22, 2023

Description

Closes hashicorp/terraform-provider-google#14447

This PR adds explicit validation feedback to users, to accompany the changes in #9014

The PR above means that "" values will be processed by all the usual provider configuration logic but the error messages returned to users may be confusing and result in GitHub issues being opened. By adding explicit validation users will be able to identify and address the problems themselves.

I've chosen to add empty string validation to the more popular fields and avoiding things like request_reason because I don't know if there's a valid use case for setting that value to an empty string or not?

Testing

I've added:

  • unit tests for the new empty string validators
  • acceptance tests showing the validators are present in the provider config and throw errors in appropriate scenarios

Here's a screenshot from a manual test. NOTE: there are always 2 errors, as both the SDK and PF validators are rejecting the bad input

Screenshot 2023-09-22 at 18 46 31

Misc details

Previously I thought that provider-level validation protecting against empty strings was a problem due to the SDK being weird/making it easy to footgun when handling zero values. After some manual tests I found this wasn't the case, so added anti-empty string validation to the SDK and PF versions of the provider config code.

NOTE: If validation exists on one version of the provider config but not the other then there's a fundamental error in the provider, as provider schemas must match when muxed together.


Release Note Template for Downstream PRs (will be copied)

provider: added provider-level validation so these fields are not set as empty strings in a user's config: `credentials`, `access_token`, `impersonate_service_account`, `project`, `billing_project`, `region`, `zone`

@SarahFrench
Copy link
Collaborator Author

Unit tests are going to fail as I haven't updated them yet, but I wanted to push this and get some acc tests running to see if this causes any problems with VCR/acc tests

@SarahFrench SarahFrench force-pushed the add-validation-to-provider-fields branch from ab01d1b to d48a1bf Compare September 22, 2023 17:00
@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@shuyama1 shuyama1 force-pushed the FEATURE-BRANCH-major-release-5.0.0 branch from 6374511 to 83df041 Compare September 22, 2023 17:58
@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 ( 25 files changed, 2232 insertions(+), 123 deletions(-))
Terraform Beta: Diff ( 25 files changed, 2232 insertions(+), 123 deletions(-))
TF Conversion: Diff ( 2 files changed, 367 insertions(+))
TF OiCS: Diff ( 1 file changed, 2 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3074
Passed tests 2766
Skipped tests: 306
Affected tests: 2

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
TestAccProviderEmptyStrings|TestAccFolderIamPolicy_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccProviderEmptyStrings[Debug log]
TestAccFolderIamPolicy_basic[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 SarahFrench force-pushed the add-validation-to-provider-fields branch from d98d059 to bcb9cad Compare September 22, 2023 20:08
@SarahFrench
Copy link
Collaborator Author

Just rebased this branch to the recent FEATURE-BRANCH-major-release-5.0.0 version

@SarahFrench
Copy link
Collaborator Author

The error messages can be seen in the debug logs for TestAccProviderEmptyStrings here

  error=
  | exit status 1
  | 
  | Error: please provide a value that isn't an empty string to this field
  | 
  |   with provider["registry.terraform.io/hashicorp/google"],
  |   on terraform_plugin_test.tf line 2, in provider "google":
  |    2: provider "google" {
  | 
  | region was set to ``
  | 
  | Error: please provide a value that isn't an empty string to this field
  | 
  |   with provider["registry.terraform.io/hashicorp/google"],
  |   on terraform_plugin_test.tf line 3, in provider "google":
  |    3: 	region = ""
  | 
  

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

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3074
Passed tests 2767
Skipped tests: 306
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
TestAccContainerNodePool_withSandboxConfig

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

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

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3074
Passed tests 2765
Skipped tests: 306
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
TestAccContainerNodePool_withSandboxConfig|TestAccDataprocClusterIamPolicy|TestAccDataSourceGoogleServiceAccountJwt

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]
TestAccDataSourceGoogleServiceAccountJwt[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:}}$
TestAccContainerNodePool_withSandboxConfig[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

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

User-supplied, empty values in the provider configuration should not be ignored and unused
3 participants