From 1b0712f2cadef1f7fd806448d6bdf5d2f0913519 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 3 Oct 2024 09:59:43 +0100 Subject: [PATCH] Fix how the plugin-framework is used in VCR tests - This carries over the idea of configuring the PF provider using the SDK provider. The changes in the MuxedProviders func mimic changes already made in main.go. - Remove unnecessary duplication of cached configs per test name; now one used regardless of PF/SDK - Updates to some DestroyProducer functions so they access the cached SDK Config struct to get a client - Remove GetFwTestProvider and the file containing it - this isn't needed. --- .../acctest/framework_test_utils.go.tmpl | 27 ----- .../terraform/acctest/provider_test_utils.go | 1 - .../terraform/acctest/test_utils.go.tmpl | 8 +- .../terraform/acctest/vcr_utils.go | 98 ++++--------------- .../framework_provider_test.go.tmpl | 20 ++-- .../terraform/fwresource/field_helpers.go | 4 +- 6 files changed, 41 insertions(+), 117 deletions(-) delete mode 100644 mmv1/third_party/terraform/acctest/framework_test_utils.go.tmpl diff --git a/mmv1/third_party/terraform/acctest/framework_test_utils.go.tmpl b/mmv1/third_party/terraform/acctest/framework_test_utils.go.tmpl deleted file mode 100644 index 4d4f113343ca..000000000000 --- a/mmv1/third_party/terraform/acctest/framework_test_utils.go.tmpl +++ /dev/null @@ -1,27 +0,0 @@ -package acctest - -import ( - "context" - "log" - "testing" - - "github.com/hashicorp/terraform-plugin-framework/diag" -) - -func GetFwTestProvider(t *testing.T) *frameworkTestProvider { - configsLock.RLock() - fwProvider, ok := fwProviders[t.Name()] - configsLock.RUnlock() - if ok { - return fwProvider - } - - var diags diag.Diagnostics - p := NewFrameworkTestProvider(t.Name()) - configureApiClient(context.Background(), &p.FrameworkProvider, &diags) - if diags.HasError() { - log.Fatalf("%d errors when configuring test provider client: first is %s", diags.ErrorsCount(), diags.Errors()[0].Detail()) - } - - return p -} diff --git a/mmv1/third_party/terraform/acctest/provider_test_utils.go b/mmv1/third_party/terraform/acctest/provider_test_utils.go index f30086539c1d..81bc0fd80a1d 100644 --- a/mmv1/third_party/terraform/acctest/provider_test_utils.go +++ b/mmv1/third_party/terraform/acctest/provider_test_utils.go @@ -20,7 +20,6 @@ var testAccProvider *schema.Provider func init() { configs = make(map[string]*transport_tpg.Config) - fwProviders = make(map[string]*frameworkTestProvider) sources = make(map[string]VcrSource) testAccProvider = provider.Provider() TestAccProviders = map[string]*schema.Provider{ diff --git a/mmv1/third_party/terraform/acctest/test_utils.go.tmpl b/mmv1/third_party/terraform/acctest/test_utils.go.tmpl index 69d453d068ef..0a11711382b8 100644 --- a/mmv1/third_party/terraform/acctest/test_utils.go.tmpl +++ b/mmv1/third_party/terraform/acctest/test_utils.go.tmpl @@ -81,9 +81,13 @@ func CheckDataSourceStateMatchesResourceStateWithIgnores(dataSourceName, resourc func MuxedProviders(testName string) (func() tfprotov5.ProviderServer, error) { ctx := context.Background() + // primary is the SDKv2 implementation of the provider + // If tests are run in VCR mode, the provider will use a cached config specific to the test name + primary := GetSDKProvider(testName) + providers := []func() tfprotov5.ProviderServer{ - providerserver.NewProtocol5(NewFrameworkTestProvider(testName)), // framework provider - GetSDKProvider(testName).GRPCProvider, // sdk provider + primary.GRPCProvider, // sdk provider + providerserver.NewProtocol5(NewFrameworkTestProvider(testName, primary)), // framework provider } muxServer, err := tf5muxserver.NewMuxServer(ctx, providers...) diff --git a/mmv1/third_party/terraform/acctest/vcr_utils.go b/mmv1/third_party/terraform/acctest/vcr_utils.go index aed2036f55cb..ba71c3fa4bb4 100644 --- a/mmv1/third_party/terraform/acctest/vcr_utils.go +++ b/mmv1/third_party/terraform/acctest/vcr_utils.go @@ -21,7 +21,6 @@ import ( "testing" "time" - "github.com/hashicorp/terraform-provider-google/google/fwmodels" "github.com/hashicorp/terraform-provider-google/google/fwprovider" tpgprovider "github.com/hashicorp/terraform-provider-google/google/provider" "github.com/hashicorp/terraform-provider-google/google/tpgresource" @@ -30,12 +29,9 @@ import ( "github.com/dnaeon/go-vcr/cassette" "github.com/dnaeon/go-vcr/recorder" - "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/datasource" - fwDiags "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/provider" - "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-go/tfprotov5" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" @@ -53,7 +49,6 @@ var configsLock = sync.RWMutex{} var sourcesLock = sync.RWMutex{} var configs map[string]*transport_tpg.Config -var fwProviders map[string]*frameworkTestProvider var sources map[string]VcrSource @@ -203,39 +198,6 @@ func closeRecorder(t *testing.T) { delete(sources, t.Name()) sourcesLock.Unlock() } - - configsLock.RLock() - fwProvider, fwOk := fwProviders[t.Name()] - configsLock.RUnlock() - if fwOk { - // We did not cache the config if it does not use VCR - if !t.Failed() && IsVcrEnabled() { - // If a test succeeds, write new seed/yaml to files - err := fwProvider.Client.Transport.(*recorder.Recorder).Stop() - if err != nil { - t.Error(err) - } - envPath := os.Getenv("VCR_PATH") - - sourcesLock.RLock() - vcrSource, ok := sources[t.Name()] - sourcesLock.RUnlock() - if ok { - err = writeSeedToFile(vcrSource.seed, vcrSeedFile(envPath, t.Name())) - if err != nil { - t.Error(err) - } - } - } - // Clean up test config - configsLock.Lock() - delete(fwProviders, t.Name()) - configsLock.Unlock() - - sourcesLock.Lock() - delete(sources, t.Name()) - sourcesLock.Unlock() - } } func isReleaseDiffEnabled() bool { @@ -319,6 +281,11 @@ func reformConfigWithProvider(config, provider string) string { return string(resourceHeader.ReplaceAll(configBytes, providerReplacementBytes)) } +// HandleVCRConfiguration configures the recorder (github.com/dnaeon/go-vcr/recorder) used in the VCR test +// This includes: +// - Setting the recording/replaying mode +// - Determining the path to the file API interactions will be recorded to/read from +// - Determining the logic used to match requests against recorded HTTP interactions (see rec.SetMatcher) func HandleVCRConfiguration(ctx context.Context, testName string, rndTripper http.RoundTripper, pollInterval time.Duration) (time.Duration, http.RoundTripper, fwDiags.Diagnostics) { var diags fwDiags.Diagnostics var vcrMode recorder.Mode @@ -377,11 +344,11 @@ func HandleVCRConfiguration(ctx context.Context, testName string, rndTripper htt if strings.Contains(contentType, "application/json") { var reqJson, cassetteJson interface{} if err := json.Unmarshal([]byte(reqBody), &reqJson); err != nil { - tflog.Debug(ctx, fmt.Sprintf("Failed to unmarshall request json: %v", err)) + tflog.Debug(ctx, fmt.Sprintf("Failed to unmarshal request json: %v", err)) return false } if err := json.Unmarshal([]byte(i.Body), &cassetteJson); err != nil { - tflog.Debug(ctx, fmt.Sprintf("Failed to unmarshall cassette json: %v", err)) + tflog.Debug(ctx, fmt.Sprintf("Failed to unmarshal cassette json: %v", err)) return false } return reflect.DeepEqual(reqJson, cassetteJson) @@ -397,18 +364,18 @@ func HandleVCRConfiguration(ctx context.Context, testName string, rndTripper htt // test versions of the provider that will call the same configure function, only append the VCR // configuration to it. -func NewFrameworkTestProvider(testName string) *frameworkTestProvider { +func NewFrameworkTestProvider(testName string, primary *schema.Provider) *frameworkTestProvider { return &frameworkTestProvider{ FrameworkProvider: fwprovider.FrameworkProvider{ Version: "test", + Primary: primary, }, TestName: testName, } } -// frameworkTestProvider is a test version of the plugin-framework version of the provider -// that embeds FrameworkProvider whose configure function we can use -// the Configure function is overwritten in the framework_provider_test file +// frameworkTestProvider is a test version of the plugin-framework version of the provider. +// This facilitates overwriting the Configure function that's used during acceptance tests. type frameworkTestProvider struct { fwprovider.FrameworkProvider TestName string @@ -416,26 +383,13 @@ type frameworkTestProvider struct { // Configure is here to overwrite the FrameworkProvider configure function for VCR testing func (p *frameworkTestProvider) Configure(ctx context.Context, req provider.ConfigureRequest, resp *provider.ConfigureResponse) { - p.FrameworkProvider.Configure(ctx, req, resp) - if IsVcrEnabled() { - if resp.Diagnostics.HasError() { - return - } - - var diags fwDiags.Diagnostics - p.PollInterval, p.Client.Transport, diags = HandleVCRConfiguration(ctx, p.TestName, p.Client.Transport, p.PollInterval) - if diags.HasError() { - resp.Diagnostics.Append(diags...) - return - } - configsLock.Lock() - fwProviders[p.TestName] = p - configsLock.Unlock() - return - } else { - tflog.Debug(ctx, "VCR_PATH or VCR_MODE not set, skipping VCR") - } + // When creating the frameworkTestProvider struct we took in a pointer to the the SDK provider. + // That SDK provider was configured using `GetSDKProvider` and `getCachedConfig`, so this framework provider will also + // use a cached client for the correct test name. + // No extra logic is required here, but in future when the SDK provider is removed this function will + // need to be updated with logic similar to that in `GetSDKProvider`. + p.FrameworkProvider.Configure(ctx, req, resp) } // DataSources overrides the provider's DataSources function so that we can append test-specific data sources to the list of data sources on the provider. @@ -446,21 +400,9 @@ func (p *frameworkTestProvider) DataSources(ctx context.Context) []func() dataso return ds } -func configureApiClient(ctx context.Context, p *fwprovider.FrameworkProvider, diags *fwDiags.Diagnostics) { - var data fwmodels.ProviderModel - var d fwDiags.Diagnostics - - // Set defaults if needed - the only attribute without a default is ImpersonateServiceAccountDelegates - // this is a bit of a hack, but we'll just initialize it here so that it's been initialized at least - data.ImpersonateServiceAccountDelegates, d = types.ListValue(types.StringType, []attr.Value{}) - diags.Append(d...) - if diags.HasError() { - return - } - p.LoadAndValidateFramework(ctx, &data, "test", diags, p.Version) -} - -// GetSDKProvider gets the SDK provider with an overwritten configure function to be called by MuxedProviders +// GetSDKProvider gets the SDK provider for use in acceptance tests +// If VCR is in use, the configure function is overwritten. +// See usage in MuxedProviders func GetSDKProvider(testName string) *schema.Provider { prov := tpgprovider.Provider() diff --git a/mmv1/third_party/terraform/fwprovider/framework_provider_test.go.tmpl b/mmv1/third_party/terraform/fwprovider/framework_provider_test.go.tmpl index 0a910cabedaf..fc9ca2f7c1e3 100644 --- a/mmv1/third_party/terraform/fwprovider/framework_provider_test.go.tmpl +++ b/mmv1/third_party/terraform/fwprovider/framework_provider_test.go.tmpl @@ -11,7 +11,6 @@ import ( "github.com/hashicorp/terraform-provider-google/google/acctest" "github.com/hashicorp/terraform-provider-google/google/fwresource" - "github.com/hashicorp/terraform-provider-google/google/fwtransport" "github.com/hashicorp/terraform-provider-google/google/tpgresource" transport_tpg "github.com/hashicorp/terraform-provider-google/google/transport" ) @@ -221,21 +220,28 @@ func testAccCheckDNSManagedZoneDestroyProducerFramework(t *testing.T) func(s *te continue } - p := acctest.GetFwTestProvider(t) + config := acctest.GoogleProviderConfig(t) - url, err := fwresource.ReplaceVarsForFrameworkTest(&p.FrameworkProvider.FrameworkProviderConfig, rs, "{{"{{"}}DNSBasePath{{"}}"}}projects/{{"{{"}}project{{"}}"}}/managedZones/{{"{{"}}name{{"}}"}}") + url, err := fwresource.ReplaceVarsForFrameworkTest(config, rs, "{{"{{"}}DNSBasePath{{"}}"}}projects/{{"{{"}}project{{"}}"}}/managedZones/{{"{{"}}name{{"}}"}}") if err != nil { return err } billingProject := "" - if !p.BillingProject.IsNull() && p.BillingProject.String() != "" { - billingProject = p.BillingProject.String() + if config.BillingProject != "" { + billingProject = config.BillingProject } - _, diags := fwtransport.SendFrameworkRequest(&p.FrameworkProvider.FrameworkProviderConfig, "GET", billingProject, url, p.UserAgent, nil) - if !diags.HasError() { + _, err = transport_tpg.SendRequest(transport_tpg.SendRequestOptions{ + Config: config, + Method: "GET", + Project: billingProject, + RawURL: url, + UserAgent: config.UserAgent, + }) + + if err == nil { return fmt.Errorf("DNSManagedZone still exists at %s", url) } } diff --git a/mmv1/third_party/terraform/fwresource/field_helpers.go b/mmv1/third_party/terraform/fwresource/field_helpers.go index cfc09b5337a7..54788d8346e7 100644 --- a/mmv1/third_party/terraform/fwresource/field_helpers.go +++ b/mmv1/third_party/terraform/fwresource/field_helpers.go @@ -9,8 +9,8 @@ import ( "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-testing/terraform" - "github.com/hashicorp/terraform-provider-google/google/fwtransport" "github.com/hashicorp/terraform-provider-google/google/tpgresource" + transport_tpg "github.com/hashicorp/terraform-provider-google/google/transport" ) // GetProject reads the "project" field from the given resource and falls @@ -69,7 +69,7 @@ func ParseProjectFieldValueFramework(resourceType, fieldValue, projectSchemaFiel // This function isn't a test of transport.go; instead, it is used as an alternative // to ReplaceVars inside tests. -func ReplaceVarsForFrameworkTest(prov *fwtransport.FrameworkProviderConfig, rs *terraform.ResourceState, linkTmpl string) (string, error) { +func ReplaceVarsForFrameworkTest(prov *transport_tpg.Config, rs *terraform.ResourceState, linkTmpl string) (string, error) { re := regexp.MustCompile("{{([[:word:]]+)}}") var project, region, zone string