Add tests around how plugin framework provider configuration code handles `credentials` values by SarahFrench · Pull Request #8801 · GoogleCloudPlatform/magic-modules

SarahFrench · 2023-08-30T14:46:57Z

This PR adds tests for how the plugin framework config code handles the credentials field's value.
These tests either:

Are the PF equivalent of existing tests for the SDK provider config code
Are tests that currently don't pass, and will be made to pass in a future PR that makes the PF behaviour mimic the SDK's behaviour:
- by ignoring empty strings
- by treating unknown values as if they were null

Note: in this PR I changed the function signature for LoadAndValidateFramework (see comment here) to enable proper testing of the function

TODOs:

Add test cases to match SDK tests
Ensure tests present about handling empty strings (will not be passing, add link to related issue)
Ensure tests present about handling Unknown values (will not be passing, add link to related issue)

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

modular-magician · 2023-08-30T14:56:49Z

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 ( 10 files changed, 161 insertions(+), 158 deletions(-))
Terraform Beta: Diff ( 10 files changed, 161 insertions(+), 158 deletions(-))

SarahFrench · 2023-08-30T15:42:26Z

 // LoadAndValidateFramework handles the bulk of configuring the provider
 // it is pulled out so that we can manually call this from our testing provider as well
-func (p *FrameworkProviderConfig) LoadAndValidateFramework(ctx context.Context, data fwmodels.ProviderModel, tfVersion string, diags *diag.Diagnostics, providerversion string) {
+func (p *FrameworkProviderConfig) LoadAndValidateFramework(ctx context.Context, data *fwmodels.ProviderModel, tfVersion string, diags *diag.Diagnostics, providerversion string) {


~~This PR isn't ready for full review yet, but~~ flagging this change.

Currently the data about the provider's configuration in the config (ie provider block) is received by the provider code and used to populate the data *fwmodels.ProviderModel struct in this code. The data struct is only used by passing it into LoadAndValidateFramework, and isn't used elsewhere. Because it's only got this one use, I think this change doesn't have much risk associated with it.

Making the function take a pointer instead of a copied value allows the test to see how the data model struct is mutated inside this function.

modular-magician · 2023-08-30T16:08:50Z

Tests analytics

Total tests: 3007
Passed tests 2709
Skipped tests: 296
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

TestAccDataprocJobIamPolicy|TestAccProjectIamPolicy_invalidMembers

Get to know how VCR tests work

modular-magician · 2023-08-30T16:17:11Z

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

… by inaccessible functions

…ig struct after `LoadAndValidateFramework` runs

…ing test case that fails

…ata model, so mutations to the data within the function change the original struct This enables tests to track how the data is mutated

…ing ADC ENV

…` now taking a pointer as an argument

modular-magician · 2023-08-31T01:23:11Z

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

modular-magician · 2023-08-31T02:34:43Z

Tests analytics

Total tests: 3003
Passed tests 2709
Skipped tests: 294
Affected tests: 0

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

modular-magician · 2023-08-31T15:03:45Z

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

modular-magician · 2023-08-31T16:15:38Z

Tests analytics

Total tests: 3008
Passed tests 2712
Skipped tests: 296
Affected tests: 0

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

SarahFrench · 2023-08-31T18:01:18Z

When the file was called mmv1/third_party/terraform/fwtransport/framework_config_test.erb it didn't get generated in the downstream at all. I totally blanked this in my PR that first introduced the file to MM, but there aren't any negative consequences beyond my tests not being present in the downstream yet.

SarahFrench · 2023-08-31T18:03:54Z

+		// Handling empty strings in config
+		// TODO(SarahFrench) make these tests pass to address: https://github.com/hashicorp/terraform-provider-google/issues/14255
+		// "when project is set as an empty string the field is treated as if it's unset, without error": {
+		// 	ConfigValues: fwmodels.ProviderModel{
+		// 		Project: types.StringValue(""),
+		// 	},
+		// 	ExpectedDataModelValue:    types.StringNull(),
+		// 	ExpectedConfigStructValue: types.StringNull(),
+		// },
+		// "when project is set as an empty string an environment variable will be used": {
+		// 	ConfigValues: fwmodels.ProviderModel{
+		// 		Project: types.StringValue(""),
+		// 	},
+		// 	EnvVariables: map[string]string{
+		// 		"GOOGLE_PROJECT": "project-from-GOOGLE_PROJECT",
+		// 	},
+		// 	ExpectedDataModelValue:    types.StringNull(),
+		// 	ExpectedConfigStructValue: types.StringValue("project-from-GOOGLE_PROJECT"),
+		// },
+		// Handling unknown values
+		// TODO(SarahFrench) make these tests pass to address: https://github.com/hashicorp/terraform-provider-google/issues/14444
+		// "when project is an unknown value, the provider treats it as if it's unset (align to SDK behaviour)": {
+		// 	ConfigValues: fwmodels.ProviderModel{
+		// 		Project: types.StringUnknown(),
+		// 	},
+		// 	ExpectedDataModelValue:    types.StringNull(),
+		// 	ExpectedConfigStructValue: types.StringNull(),
+		// },


I'll make these tests pass in a subsequent PR, for ease of review and to ensure that all these 'fixes' can be done at once later in a single PR instead of across multiple PRs

…o.erb Co-authored-by: Riley Karson <rileykarson@google.com>

modular-magician · 2023-09-06T19:10:25Z

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

modular-magician · 2023-09-06T20:22:51Z

Tests analytics

Total tests: 3014
Passed tests 2717
Skipped tests: 296
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

TestAccDataprocClusterIamPolicy

Get to know how VCR tests work

modular-magician · 2023-09-06T20:29:20Z

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

…dles `credentials` values (GoogleCloudPlatform#8801) * Add initial version of plugin framework provider config test affected by inaccessible functions * Refactor provider config tests to use plugin-framework types * Add test case about handling of Unknown values for `project` * Update tests to check values in BOTH the data model and provider config struct after `LoadAndValidateFramework` runs * Add some tests for `credentials` in plugin framework provider, including test case that fails * Update `LoadAndValidateFramework` to take a pointer to the provider data model, so mutations to the data within the function change the original struct This enables tests to track how the data is mutated * Add remaining `credentials` test cases to check PF/SDK config parity * Make tests unset ADC ENV automatically, update comments to tests setting ADC ENV * Add test for behaviour when credentials value is unknown * Add comment referring devs to where unknown value test is implemented * Remove duplicated test case * Fix filename so it's generated correctly * Remove fmt line * Update `project` tests that are affected by `LoadAndValidateFramework` now taking a pointer as an argument * Update mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb Co-authored-by: Riley Karson <rileykarson@google.com> --------- Co-authored-by: Riley Karson <rileykarson@google.com>

SarahFrench changed the base branch from main to address-circular-dependencies-pf-config-tests August 30, 2023 14:47

SarahFrench commented Aug 30, 2023

View reviewed changes

SarahFrench added 9 commits August 31, 2023 01:00

Add initial version of plugin framework provider config test affected…

94f95dc

… by inaccessible functions

Refactor provider config tests to use plugin-framework types

993b808

Add test case about handling of Unknown values for project

1aaace5

Update tests to check values in BOTH the data model and provider conf…

85192ed

…ig struct after `LoadAndValidateFramework` runs

Add some tests for credentials in plugin framework provider, includ…

3d8c55e

…ing test case that fails

Update LoadAndValidateFramework to take a pointer to the provider d…

9a53151

…ata model, so mutations to the data within the function change the original struct This enables tests to track how the data is mutated

Add remaining credentials test cases to check PF/SDK config parity

1a0c81a

Make tests unset ADC ENV automatically, update comments to tests sett…

07a7466

…ing ADC ENV

Add test for behaviour when credentials value is unknown

8b1f805

SarahFrench force-pushed the add-pf-credentials-tests branch from a450ef8 to 8b1f805 Compare August 31, 2023 00:03

SarahFrench changed the base branch from address-circular-dependencies-pf-config-tests to main August 31, 2023 00:05

SarahFrench marked this pull request as ready for review August 31, 2023 00:08

This comment was marked as outdated.

Sign in to view

Add comment referring devs to where unknown value test is implemented

39615d6

This comment was marked as outdated.

Sign in to view

Remove duplicated test case

c25c3a6

This comment was marked as outdated.

Sign in to view

Fix filename so it's generated correctly

42f4daa

This comment was marked as outdated.

Sign in to view

SarahFrench added 2 commits August 31, 2023 02:12

Remove fmt line

d42fd6a

Update project tests that are affected by `LoadAndValidateFramework…

3854ebd

…` now taking a pointer as an argument

SarahFrench marked this pull request as draft August 31, 2023 01:15

SarahFrench mentioned this pull request Aug 31, 2023

Add tests around how plugin framework provider configuration code handles impersonate_service_account, impersonate_service_account_delegates values #8818

Merged

7 tasks

SarahFrench mentioned this pull request Aug 31, 2023

Add tests around how plugin framework provider configuration code handles access_token values #8814

Merged

4 tasks

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

SarahFrench marked this pull request as ready for review August 31, 2023 17:59

SarahFrench requested a review from rileykarson August 31, 2023 17:59

SarahFrench commented Aug 31, 2023

View reviewed changes

SarahFrench requested review from rileykarson and zli82016 and removed request for rileykarson September 5, 2023 15:31

SarahFrench mentioned this pull request Sep 6, 2023

Add tests around how plugin framework provider configuration code handles scopes values #8874

Merged

3 tasks

rileykarson reviewed Sep 6, 2023

View reviewed changes

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

rileykarson approved these changes Sep 6, 2023

View reviewed changes

Update mmv1/third_party/terraform/fwtransport/framework_config_test.g…

8940c96

…o.erb Co-authored-by: Riley Karson <rileykarson@google.com>

SarahFrench merged commit 356bf50 into main Sep 6, 2023

SarahFrench deleted the add-pf-credentials-tests branch September 11, 2023 11:45

Conversation

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

Uh oh!

TODOs:

Uh oh!

modular-magician commented Aug 30, 2023

Diff report

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modular-magician commented Aug 30, 2023

Tests analytics

Action taken

Uh oh!

modular-magician commented Aug 30, 2023

Rerun these tests in REPLAYING mode to catch issues

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

modular-magician commented Aug 31, 2023

Diff report

Uh oh!

modular-magician commented Aug 31, 2023

Tests analytics

Uh oh!

This comment was marked as resolved.

This comment was marked as outdated.

modular-magician commented Aug 31, 2023

Diff report

Uh oh!

modular-magician commented Aug 31, 2023

Tests analytics

Uh oh!

SarahFrench Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

SarahFrench Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

modular-magician commented Sep 6, 2023

Diff report

Uh oh!

modular-magician commented Sep 6, 2023

Tests analytics

Action taken

Uh oh!

modular-magician commented Sep 6, 2023

Rerun these tests in REPLAYING mode to catch issues

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SarahFrench commented Aug 30, 2023 •

edited

Loading

SarahFrench Aug 30, 2023 •

edited

Loading