From a858e02833a99687bdded960f0a911c7dddeec60 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 8 Nov 2022 11:00:13 -0500 Subject: [PATCH] helper/resource: Check for existing provider configuration block in TestStep merged configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reference: https://github.com/hashicorp/terraform-plugin-sdk/issues/1064 Previously: ``` --- FAIL: TestStepMergedConfig/teststep-externalproviders-config-with-provider-block-unquoted (0.00s) /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/resource/teststep_providers_test.go:643: unexpected difference:   (    """    ... // 6 identical lines    }    } -  -  provider "test" {} -        provider test {}    ... // 2 identical lines    """   ) --- FAIL: TestStepMergedConfig/teststep-externalproviders-config-with-provider-block-quoted (0.00s) /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/resource/teststep_providers_test.go:643: unexpected difference:   (    """    ... // 6 identical lines    }    } -  -  provider "test" {} -        provider "test" {}    ... // 2 identical lines    """   ) ``` --- .changelog/pending.txt | 3 + helper/resource/testcase_providers.go | 6 +- helper/resource/testcase_providers_test.go | 28 ++- helper/resource/testing_new.go | 6 +- helper/resource/teststep_providers.go | 37 +++- helper/resource/teststep_providers_test.go | 235 ++++++++++++++++++++- 6 files changed, 296 insertions(+), 19 deletions(-) create mode 100644 .changelog/pending.txt diff --git a/.changelog/pending.txt b/.changelog/pending.txt new file mode 100644 index 00000000000..a486e261174 --- /dev/null +++ b/.changelog/pending.txt @@ -0,0 +1,3 @@ +```release-note:bug +helper/resource: Prevented provider configuration already given error when `TestStep` type `Config` field already contained provider configuration block +``` diff --git a/helper/resource/testcase_providers.go b/helper/resource/testcase_providers.go index c09e4657cc2..19a548cdb1f 100644 --- a/helper/resource/testcase_providers.go +++ b/helper/resource/testcase_providers.go @@ -9,7 +9,7 @@ import ( // providerConfig takes the list of providers in a TestCase and returns a // config with only empty provider blocks. This is useful for Import, where no // config is provided, but the providers must be defined. -func (c TestCase) providerConfig(_ context.Context) string { +func (c TestCase) providerConfig(_ context.Context, skipProviderBlock bool) string { var providerBlocks, requiredProviderBlocks strings.Builder // [BF] The Providers field handling predates the logic being moved to this @@ -21,7 +21,9 @@ func (c TestCase) providerConfig(_ context.Context) string { } for name, externalProvider := range c.ExternalProviders { - providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + if !skipProviderBlock { + providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + } if externalProvider.Source == "" && externalProvider.VersionConstraint == "" { continue diff --git a/helper/resource/testcase_providers_test.go b/helper/resource/testcase_providers_test.go index c52ebf8bb8e..5bedd7fea52 100644 --- a/helper/resource/testcase_providers_test.go +++ b/helper/resource/testcase_providers_test.go @@ -17,8 +17,9 @@ func TestTestCaseProviderConfig(t *testing.T) { t.Parallel() tests := map[string]struct { - testCase TestCase - expected string + testCase TestCase + skipProviderBlock bool + expected string }{ "externalproviders-and-protov5providerfactories": { testCase: TestCase{ @@ -103,6 +104,27 @@ provider "externaltest" {} }, expected: `provider "test" {}`, }, + "externalproviders-skip-provider-block": { + testCase: TestCase{ + ExternalProviders: map[string]ExternalProvider{ + "test": { + Source: "registry.terraform.io/hashicorp/test", + VersionConstraint: "1.2.3", + }, + }, + }, + skipProviderBlock: true, + expected: ` +terraform { + required_providers { + test = { + source = "registry.terraform.io/hashicorp/test" + version = "1.2.3" + } + } +} +`, + }, "externalproviders-source-and-versionconstraint": { testCase: TestCase{ ExternalProviders: map[string]ExternalProvider{ @@ -205,7 +227,7 @@ provider "test" {} t.Run(name, func(t *testing.T) { t.Parallel() - got := test.testCase.providerConfig(context.Background()) + got := test.testCase.providerConfig(context.Background(), test.skipProviderBlock) if diff := cmp.Diff(strings.TrimSpace(got), strings.TrimSpace(test.expected)); diff != "" { t.Errorf("unexpected difference: %s", diff) diff --git a/helper/resource/testing_new.go b/helper/resource/testing_new.go index 6fa78c23ec7..007ab66c0d8 100644 --- a/helper/resource/testing_new.go +++ b/helper/resource/testing_new.go @@ -89,7 +89,7 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest }() if c.hasProviders(ctx) { - err := wd.SetConfig(ctx, c.providerConfig(ctx)) + err := wd.SetConfig(ctx, c.providerConfig(ctx, false)) if err != nil { logging.HelperResourceError(ctx, @@ -170,7 +170,7 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest protov6: protov6ProviderFactories(c.ProtoV6ProviderFactories).merge(step.ProtoV6ProviderFactories), } - providerCfg := step.providerConfig(ctx) + providerCfg := step.providerConfig(ctx, step.configHasProviderBlock(ctx)) err := wd.SetConfig(ctx, providerCfg) @@ -371,7 +371,7 @@ func testIDRefresh(ctx context.Context, t testing.T, c TestCase, wd *plugintest. // Temporarily set the config to a minimal provider config for the refresh // test. After the refresh we can reset it. - err := wd.SetConfig(ctx, c.providerConfig(ctx)) + err := wd.SetConfig(ctx, c.providerConfig(ctx, step.configHasProviderBlock(ctx))) if err != nil { t.Fatalf("Error setting import test config: %s", err) } diff --git a/helper/resource/teststep_providers.go b/helper/resource/teststep_providers.go index b3c71b8c010..757f8f638cf 100644 --- a/helper/resource/teststep_providers.go +++ b/helper/resource/teststep_providers.go @@ -3,9 +3,24 @@ package resource import ( "context" "fmt" + "regexp" "strings" ) +var configProviderBlockRegex = regexp.MustCompile(`provider "?[a-zA-Z0-9_-]+"? {`) + +// configHasProviderBlock returns true if the Config has declared a provider +// configuration block, e.g. provider "examplecloud" {...} +func (s TestStep) configHasProviderBlock(_ context.Context) bool { + return configProviderBlockRegex.MatchString(s.Config) +} + +// configHasTerraformBlock returns true if the Config has declared a terraform +// configuration block, e.g. terraform {...} +func (s TestStep) configHasTerraformBlock(_ context.Context) bool { + return strings.Contains(s.Config, "terraform {") +} + // mergedConfig prepends any necessary terraform configuration blocks to the // TestStep Config. // @@ -18,12 +33,16 @@ func (s TestStep) mergedConfig(ctx context.Context, testCase TestCase) string { // Prevent issues with existing configurations containing the terraform // configuration block. - if !strings.Contains(s.Config, "terraform {") { - if testCase.hasProviders(ctx) { - config.WriteString(testCase.providerConfig(ctx)) - } else { - config.WriteString(s.providerConfig(ctx)) - } + if s.configHasTerraformBlock(ctx) { + config.WriteString(s.Config) + + return config.String() + } + + if testCase.hasProviders(ctx) { + config.WriteString(testCase.providerConfig(ctx, s.configHasProviderBlock(ctx))) + } else { + config.WriteString(s.providerConfig(ctx, s.configHasProviderBlock(ctx))) } config.WriteString(s.Config) @@ -34,11 +53,13 @@ func (s TestStep) mergedConfig(ctx context.Context, testCase TestCase) string { // providerConfig takes the list of providers in a TestStep and returns a // config with only empty provider blocks. This is useful for Import, where no // config is provided, but the providers must be defined. -func (s TestStep) providerConfig(_ context.Context) string { +func (s TestStep) providerConfig(_ context.Context, skipProviderBlock bool) string { var providerBlocks, requiredProviderBlocks strings.Builder for name, externalProvider := range s.ExternalProviders { - providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + if !skipProviderBlock { + providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + } if externalProvider.Source == "" && externalProvider.VersionConstraint == "" { continue diff --git a/helper/resource/teststep_providers_test.go b/helper/resource/teststep_providers_test.go index 09ed0bfb310..a979b16c8e3 100644 --- a/helper/resource/teststep_providers_test.go +++ b/helper/resource/teststep_providers_test.go @@ -18,6 +18,114 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) +func TestStepConfigHasProviderBlock(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + testStep TestStep + expected bool + }{ + "no-config": { + testStep: TestStep{}, + expected: false, + }, + "provider-meta-attribute": { + testStep: TestStep{ + Config: ` +resource "test_test" "test" { + provider = test.test +} +`, + }, + expected: false, + }, + "provider-object-attribute": { + testStep: TestStep{ + Config: ` +resource "test_test" "test" { + test = { + provider = { + test = true + } + } +} +`, + }, + expected: false, + }, + "provider-string-attribute": { + testStep: TestStep{ + Config: ` +resource "test_test" "test" { + test = { + provider = "test" + } +} +`, + }, + expected: false, + }, + "provider-block-quoted-with-attributes": { + testStep: TestStep{ + Config: ` +provider "test" { + test = true +} + +resource "test_test" "test" {} +`, + }, + expected: true, + }, + "provider-block-unquoted-with-attributes": { + testStep: TestStep{ + Config: ` +provider test { + test = true +} + +resource "test_test" "test" {} +`, + }, + expected: true, + }, + "provider-block-quoted-without-attributes": { + testStep: TestStep{ + Config: ` +provider "test" {} + +resource "test_test" "test" {} +`, + }, + expected: true, + }, + "provider-block-unquoted-without-attributes": { + testStep: TestStep{ + Config: ` +provider test {} + +resource "test_test" "test" {} +`, + }, + expected: true, + }, + } + + for name, testCase := range testCases { + name, testCase := name, testCase + + t.Run(name, func(t *testing.T) { + t.Parallel() + + got := testCase.testStep.configHasProviderBlock(context.Background()) + + if testCase.expected != got { + t.Errorf("expected %t, got %t", testCase.expected, got) + } + }) + } +} + func TestStepMergedConfig(t *testing.T) { t.Parallel() @@ -391,6 +499,105 @@ provider "externaltest" {} resource "externaltest_test" "test" {} resource "localtest_test" "test" {} +`, + }, + "teststep-externalproviders-config-with-provider-block-quoted": { + testCase: TestCase{}, + testStep: TestStep{ + ExternalProviders: map[string]ExternalProvider{ + "test": { + Source: "registry.terraform.io/hashicorp/test", + VersionConstraint: "1.2.3", + }, + }, + Config: ` +provider "test" {} + +resource "test_test" "test" {} +`, + }, + expected: ` +terraform { + required_providers { + test = { + source = "registry.terraform.io/hashicorp/test" + version = "1.2.3" + } + } +} + + + +provider "test" {} + +resource "test_test" "test" {} +`, + }, + "teststep-externalproviders-config-with-provider-block-unquoted": { + testCase: TestCase{}, + testStep: TestStep{ + ExternalProviders: map[string]ExternalProvider{ + "test": { + Source: "registry.terraform.io/hashicorp/test", + VersionConstraint: "1.2.3", + }, + }, + Config: ` +provider test {} + +resource "test_test" "test" {} +`, + }, + expected: ` +terraform { + required_providers { + test = { + source = "registry.terraform.io/hashicorp/test" + version = "1.2.3" + } + } +} + + + +provider test {} + +resource "test_test" "test" {} +`, + }, + "teststep-externalproviders-config-with-terraform-block": { + testCase: TestCase{}, + testStep: TestStep{ + ExternalProviders: map[string]ExternalProvider{ + "test": { + Source: "registry.terraform.io/hashicorp/test", + VersionConstraint: "1.2.3", + }, + }, + Config: ` +terraform { + required_providers { + test = { + source = "registry.terraform.io/hashicorp/test" + version = "1.2.3" + } + } +} + +resource "test_test" "test" {} +`, + }, + expected: ` +terraform { + required_providers { + test = { + source = "registry.terraform.io/hashicorp/test" + version = "1.2.3" + } + } +} + +resource "test_test" "test" {} `, }, "teststep-externalproviders-missing-source-and-versionconstraint": { @@ -555,8 +762,9 @@ func TestStepProviderConfig(t *testing.T) { t.Parallel() testCases := map[string]struct { - testStep TestStep - expected string + testStep TestStep + skipProviderBlock bool + expected string }{ "externalproviders-and-protov5providerfactories": { testStep: TestStep{ @@ -641,6 +849,27 @@ provider "externaltest" {} }, expected: `provider "test" {}`, }, + "externalproviders-skip-provider-block": { + testStep: TestStep{ + ExternalProviders: map[string]ExternalProvider{ + "test": { + Source: "registry.terraform.io/hashicorp/test", + VersionConstraint: "1.2.3", + }, + }, + }, + skipProviderBlock: true, + expected: ` +terraform { + required_providers { + test = { + source = "registry.terraform.io/hashicorp/test" + version = "1.2.3" + } + } +} +`, + }, "externalproviders-source-and-versionconstraint": { testStep: TestStep{ ExternalProviders: map[string]ExternalProvider{ @@ -735,7 +964,7 @@ provider "test" {} t.Run(name, func(t *testing.T) { t.Parallel() - got := testCase.testStep.providerConfig(context.Background()) + got := testCase.testStep.providerConfig(context.Background(), testCase.skipProviderBlock) if diff := cmp.Diff(strings.TrimSpace(got), strings.TrimSpace(testCase.expected)); diff != "" { t.Errorf("unexpected difference: %s", diff)