helper/resource: Check for existing provider configuration block in T…

…estStep merged configuration Reference: #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 """ ) ```
hashicorp · Nov 8, 2022 · a858e02 · a858e02
1 parent 7bc4c6e
commit a858e02
Show file tree

Hide file tree

Showing 6 changed files with 296 additions and 19 deletions.
diff --git a/.changelog/pending.txt 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
@@ -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
@@ -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
@@ -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
@@ -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