From 48fc2d3ac7c24cb9e29a54b370de858e0c98f0cc Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Thu, 16 May 2024 13:15:20 +0200 Subject: [PATCH] terraform test: Disallow version constraints within test files (#35161) --- internal/command/init_test.go | 40 +++++++++++++++++++ internal/command/providers_test.go | 1 - .../main.tf | 3 ++ .../main.tftest.hcl | 25 ++++++++++++ .../setup/main.tf | 11 +++++ .../testdata/providers/tests/main.tftest.hcl | 2 + internal/configs/config.go | 23 +++-------- internal/configs/config_test.go | 40 +++++-------------- internal/configs/parser_config.go | 2 +- internal/configs/provider.go | 29 +++++++++----- internal/configs/test_file.go | 2 +- .../provider-reqs-root.tftest.hcl | 8 ++-- .../provider-reqs-with-tests/setup/setup.tf | 2 + internal/moduletest/config/config.go | 2 - 14 files changed, 124 insertions(+), 66 deletions(-) create mode 100644 internal/command/testdata/init-with-tests-external-providers/main.tf create mode 100644 internal/command/testdata/init-with-tests-external-providers/main.tftest.hcl create mode 100644 internal/command/testdata/init-with-tests-external-providers/setup/main.tf diff --git a/internal/command/init_test.go b/internal/command/init_test.go index 01b35ba5fd..5f51b1bc45 100644 --- a/internal/command/init_test.go +++ b/internal/command/init_test.go @@ -25,6 +25,8 @@ import ( "github.com/hashicorp/terraform/internal/depsfile" "github.com/hashicorp/terraform/internal/getproviders" "github.com/hashicorp/terraform/internal/providercache" + "github.com/hashicorp/terraform/internal/providers" + testing_provider "github.com/hashicorp/terraform/internal/providers/testing" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/states/statefile" "github.com/hashicorp/terraform/internal/states/statemgr" @@ -2831,6 +2833,44 @@ func TestInit_invalidSyntaxBackendAttribute(t *testing.T) { } } +func TestInit_testsWithExternalProviders(t *testing.T) { + td := t.TempDir() + testCopyDir(t, testFixturePath("init-with-tests-external-providers"), td) + defer testChdir(t, td)() + + providerSource, close := newMockProviderSource(t, map[string][]string{ + "hashicorp/testing": {"1.0.0"}, + "testing/configure": {"1.0.0"}, + }) + defer close() + + hashicorpTestingProviderAddress := addrs.NewDefaultProvider("testing") + hashicorpTestingProvider := new(testing_provider.MockProvider) + testingConfigureProviderAddress := addrs.NewProvider(addrs.DefaultProviderRegistryHost, "testing", "configure") + testingConfigureProvider := new(testing_provider.MockProvider) + + ui := new(cli.MockUi) + view, done := testView(t) + c := &InitCommand{ + Meta: Meta{ + testingOverrides: &testingOverrides{ + Providers: map[addrs.Provider]providers.Factory{ + hashicorpTestingProviderAddress: providers.FactoryFixed(hashicorpTestingProvider), + testingConfigureProviderAddress: providers.FactoryFixed(testingConfigureProvider), + }, + }, + Ui: ui, + View: view, + ProviderSource: providerSource, + }, + } + + var args []string + if code := c.Run(args); code != 0 { + t.Fatalf("bad: \n%s", done(t).All()) + } +} + func TestInit_tests(t *testing.T) { // Create a temporary working directory that is empty td := t.TempDir() diff --git a/internal/command/providers_test.go b/internal/command/providers_test.go index 98c0c1fe19..8335d92120 100644 --- a/internal/command/providers_test.go +++ b/internal/command/providers_test.go @@ -192,7 +192,6 @@ func TestProviders_tests(t *testing.T) { } wantOutput := []string{ - "provider[registry.terraform.io/hashicorp/foo]", "test.main", "provider[registry.terraform.io/hashicorp/bar]", } diff --git a/internal/command/testdata/init-with-tests-external-providers/main.tf b/internal/command/testdata/init-with-tests-external-providers/main.tf new file mode 100644 index 0000000000..0bc133ee12 --- /dev/null +++ b/internal/command/testdata/init-with-tests-external-providers/main.tf @@ -0,0 +1,3 @@ +resource "testing_instance" "baz" { + ami = "baz" +} diff --git a/internal/command/testdata/init-with-tests-external-providers/main.tftest.hcl b/internal/command/testdata/init-with-tests-external-providers/main.tftest.hcl new file mode 100644 index 0000000000..3ba7e33205 --- /dev/null +++ b/internal/command/testdata/init-with-tests-external-providers/main.tftest.hcl @@ -0,0 +1,25 @@ + +// configure is not a "hashicorp" provider, so it won't be able to load +// this using the default behaviour. Terraform will need to look into the setup +// module to find the provider configuration. +provider "configure" {} + +// testing is a "hashicorp" provider, so it can load this using the defaults +// even though not required provider block providers a definition for it. +provider "testing" {} + +run "setup" { + module { + source = "./setup" + } + + providers = { + configure = configure + } +} + +run "test" { + providers = { + testing = testing + } +} diff --git a/internal/command/testdata/init-with-tests-external-providers/setup/main.tf b/internal/command/testdata/init-with-tests-external-providers/setup/main.tf new file mode 100644 index 0000000000..607cfe131a --- /dev/null +++ b/internal/command/testdata/init-with-tests-external-providers/setup/main.tf @@ -0,0 +1,11 @@ +terraform { + required_providers { + configure = { + source = "testing/configure" + } + } +} + +resource "configure_instance" "baz" { + ami = "baz" +} diff --git a/internal/command/testdata/providers/tests/main.tftest.hcl b/internal/command/testdata/providers/tests/main.tftest.hcl index 08de6964a1..17b46b3340 100644 --- a/internal/command/testdata/providers/tests/main.tftest.hcl +++ b/internal/command/testdata/providers/tests/main.tftest.hcl @@ -1,3 +1,5 @@ +// This won't actually show up in the providers list, as nothing is actually +// using it. provider "foo" { } diff --git a/internal/configs/config.go b/internal/configs/config.go index 86a8118665..25c0e292fb 100644 --- a/internal/configs/config.go +++ b/internal/configs/config.go @@ -384,10 +384,6 @@ func (c *Config) ProviderRequirementsByModule() (*ModuleRequirements, hcl.Diagno Runs: make(map[string]*ModuleRequirements), } - for _, provider := range test.Providers { - diags = append(diags, c.addProviderRequirementsFromProviderBlock(testReqs.Requirements, provider)...) - } - for _, run := range test.Runs { if run.ConfigUnderTest == nil { continue @@ -556,20 +552,13 @@ func (c *Config) addProviderRequirements(reqs providerreqs.Requirements, recurse // We may have provider blocks and required_providers set in some testing // files. - if tests { + if tests && recurse { for _, file := range c.Module.Tests { - for _, provider := range file.Providers { - moreDiags := c.addProviderRequirementsFromProviderBlock(reqs, provider) - diags = append(diags, moreDiags...) - } - - if recurse { - // Then we'll also look for requirements in testing modules. - for _, run := range file.Runs { - if run.ConfigUnderTest != nil { - moreDiags := run.ConfigUnderTest.addProviderRequirements(reqs, true, false) - diags = append(diags, moreDiags...) - } + // Then we'll also look for requirements in testing modules. + for _, run := range file.Runs { + if run.ConfigUnderTest != nil { + moreDiags := run.ConfigUnderTest.addProviderRequirements(reqs, true, false) + diags = append(diags, moreDiags...) } } } diff --git a/internal/configs/config_test.go b/internal/configs/config_test.go index 9441861286..b8e374d72e 100644 --- a/internal/configs/config_test.go +++ b/internal/configs/config_test.go @@ -165,12 +165,7 @@ func TestConfigProviderRequirements(t *testing.T) { func TestConfigProviderRequirementsInclTests(t *testing.T) { cfg, diags := testNestedModuleConfigFromDirWithTests(t, "testdata/provider-reqs-with-tests") - // TODO: Version Constraint Deprecation. - // Once we've removed the version argument from provider configuration - // blocks, this can go back to expected 0 diagnostics. - // assertNoDiagnostics(t, diags) - assertDiagnosticCount(t, diags, 1) - assertDiagnosticSummary(t, diags, "Version constraints inside provider configuration blocks are deprecated") + assertDiagnosticCount(t, diags, 0) tlsProvider := addrs.NewProvider( addrs.DefaultProviderRegistryHost, @@ -189,7 +184,7 @@ func TestConfigProviderRequirementsInclTests(t *testing.T) { nullProvider: providerreqs.MustParseVersionConstraints("~> 2.0.0"), randomProvider: providerreqs.MustParseVersionConstraints("~> 1.2.0"), tlsProvider: providerreqs.MustParseVersionConstraints("~> 3.0"), - configuredProvider: providerreqs.MustParseVersionConstraints("~> 1.4"), + configuredProvider: nil, impliedProvider: nil, terraformProvider: nil, } @@ -243,12 +238,7 @@ func TestConfigProviderRequirementsShallow(t *testing.T) { func TestConfigProviderRequirementsShallowInclTests(t *testing.T) { cfg, diags := testNestedModuleConfigFromDirWithTests(t, "testdata/provider-reqs-with-tests") - // TODO: Version Constraint Deprecation. - // Once we've removed the version argument from provider configuration - // blocks, this can go back to expected 0 diagnostics. - // assertNoDiagnostics(t, diags) - assertDiagnosticCount(t, diags, 1) - assertDiagnosticSummary(t, diags, "Version constraints inside provider configuration blocks are deprecated") + assertDiagnosticCount(t, diags, 0) tlsProvider := addrs.NewProvider( addrs.DefaultProviderRegistryHost, @@ -256,15 +246,13 @@ func TestConfigProviderRequirementsShallowInclTests(t *testing.T) { ) impliedProvider := addrs.NewDefaultProvider("implied") terraformProvider := addrs.NewBuiltInProvider("terraform") - configuredProvider := addrs.NewDefaultProvider("configured") got, diags := cfg.ProviderRequirementsShallow() assertNoDiagnostics(t, diags) want := providerreqs.Requirements{ - tlsProvider: providerreqs.MustParseVersionConstraints("~> 3.0"), - configuredProvider: providerreqs.MustParseVersionConstraints("~> 1.4"), - impliedProvider: nil, - terraformProvider: nil, + tlsProvider: providerreqs.MustParseVersionConstraints("~> 3.0"), + impliedProvider: nil, + terraformProvider: nil, } if diff := cmp.Diff(want, got); diff != "" { @@ -346,12 +334,7 @@ func TestConfigProviderRequirementsByModule(t *testing.T) { func TestConfigProviderRequirementsByModuleInclTests(t *testing.T) { cfg, diags := testNestedModuleConfigFromDirWithTests(t, "testdata/provider-reqs-with-tests") - // TODO: Version Constraint Deprecation. - // Once we've removed the version argument from provider configuration - // blocks, this can go back to expected 0 diagnostics. - // assertNoDiagnostics(t, diags) - assertDiagnosticCount(t, diags, 1) - assertDiagnosticSummary(t, diags, "Version constraints inside provider configuration blocks are deprecated") + assertDiagnosticCount(t, diags, 0) tlsProvider := addrs.NewProvider( addrs.DefaultProviderRegistryHost, @@ -378,17 +361,16 @@ func TestConfigProviderRequirementsByModuleInclTests(t *testing.T) { Children: make(map[string]*ModuleRequirements), Tests: map[string]*TestFileModuleRequirements{ "provider-reqs-root.tftest.hcl": { - Requirements: providerreqs.Requirements{ - configuredProvider: providerreqs.MustParseVersionConstraints("~> 1.4"), - }, + Requirements: providerreqs.Requirements{}, Runs: map[string]*ModuleRequirements{ "setup": { Name: "setup", SourceAddr: addrs.ModuleSourceLocal("./setup"), SourceDir: "testdata/provider-reqs-with-tests/setup", Requirements: providerreqs.Requirements{ - nullProvider: providerreqs.MustParseVersionConstraints("~> 2.0.0"), - randomProvider: providerreqs.MustParseVersionConstraints("~> 1.2.0"), + nullProvider: providerreqs.MustParseVersionConstraints("~> 2.0.0"), + randomProvider: providerreqs.MustParseVersionConstraints("~> 1.2.0"), + configuredProvider: nil, }, Children: make(map[string]*ModuleRequirements), Tests: make(map[string]*TestFileModuleRequirements), diff --git a/internal/configs/parser_config.go b/internal/configs/parser_config.go index 09fe677df5..83148bfe31 100644 --- a/internal/configs/parser_config.go +++ b/internal/configs/parser_config.go @@ -147,7 +147,7 @@ func parseConfigFile(body hcl.Body, diags hcl.Diagnostics, override, allowExperi }) case "provider": - cfg, cfgDiags := decodeProviderBlock(block) + cfg, cfgDiags := decodeProviderBlock(block, false) diags = append(diags, cfgDiags...) if cfg != nil { file.ProviderConfigs = append(file.ProviderConfigs, cfg) diff --git a/internal/configs/provider.go b/internal/configs/provider.go index 49d51d6a2f..9bea8c4cd9 100644 --- a/internal/configs/provider.go +++ b/internal/configs/provider.go @@ -48,7 +48,7 @@ type Provider struct { MockDataExternalSource string } -func decodeProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) { +func decodeProviderBlock(block *hcl.Block, testFile bool) (*Provider, hcl.Diagnostics) { var diags hcl.Diagnostics content, config, moreDiags := block.Body.PartialContent(providerBlockSchema) @@ -92,15 +92,24 @@ func decodeProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) { } if attr, exists := content.Attributes["version"]; exists { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Version constraints inside provider configuration blocks are deprecated", - Detail: "Terraform 0.13 and earlier allowed provider version constraints inside the provider configuration block, but that is now deprecated and will be removed in a future version of Terraform. To silence this warning, move the provider version constraint into the required_providers block.", - Subject: attr.Expr.Range().Ptr(), - }) - var versionDiags hcl.Diagnostics - provider.Version, versionDiags = decodeVersionConstraint(attr) - diags = append(diags, versionDiags...) + if testFile { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Version constraints are not allowed in test files", + Detail: "Version constraints inside provider configuration blocks are not allowed in test files. To silence this error, move the provider version constraint into the required_providers block of the configuration that uses this provider.", + Subject: attr.Expr.Range().Ptr(), + }) + } else { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Version constraints inside provider configuration blocks are deprecated", + Detail: "Terraform 0.13 and earlier allowed provider version constraints inside the provider configuration block, but that is now deprecated and will be removed in a future version of Terraform. To silence this warning, move the provider version constraint into the required_providers block.", + Subject: attr.Expr.Range().Ptr(), + }) + var versionDiags hcl.Diagnostics + provider.Version, versionDiags = decodeVersionConstraint(attr) + diags = append(diags, versionDiags...) + } } // Reserved attribute names diff --git a/internal/configs/test_file.go b/internal/configs/test_file.go index 5558490d69..0a9cd7bca3 100644 --- a/internal/configs/test_file.go +++ b/internal/configs/test_file.go @@ -337,7 +337,7 @@ func loadTestFile(body hcl.Body) (*TestFile, hcl.Diagnostics) { tf.Variables[v.Name] = v.Expr } case "provider": - provider, providerDiags := decodeProviderBlock(block) + provider, providerDiags := decodeProviderBlock(block, true) diags = append(diags, providerDiags...) if provider != nil { key := provider.moduleUniqueKey() diff --git a/internal/configs/testdata/provider-reqs-with-tests/provider-reqs-root.tftest.hcl b/internal/configs/testdata/provider-reqs-with-tests/provider-reqs-root.tftest.hcl index e368ef00da..5f52a23a70 100644 --- a/internal/configs/testdata/provider-reqs-with-tests/provider-reqs-root.tftest.hcl +++ b/internal/configs/testdata/provider-reqs-with-tests/provider-reqs-root.tftest.hcl @@ -1,8 +1,6 @@ -# There is no provider in required_providers called "configured", so the version -# constraint should come from this configuration block. -provider "configured" { - version = "~> 1.4" -} +# There is no provider in required_providers called "configured", so we won't +# have a version constraint for it. +provider "configured" {} run "setup" { module { diff --git a/internal/configs/testdata/provider-reqs-with-tests/setup/setup.tf b/internal/configs/testdata/provider-reqs-with-tests/setup/setup.tf index 7341d6af93..ce7f37e4d8 100644 --- a/internal/configs/testdata/provider-reqs-with-tests/setup/setup.tf +++ b/internal/configs/testdata/provider-reqs-with-tests/setup/setup.tf @@ -6,3 +6,5 @@ terraform { } } } + +resource "configured_resource" "resource" {} diff --git a/internal/moduletest/config/config.go b/internal/moduletest/config/config.go index 71c6527d46..292582aac1 100644 --- a/internal/moduletest/config/config.go +++ b/internal/moduletest/config/config.go @@ -87,7 +87,6 @@ func TransformConfigForTest(config *configs.Config, run *moduletest.Run, file *m NameRange: ref.InChild.NameRange, Alias: ref.InChild.Alias, AliasRange: ref.InChild.AliasRange, - Version: testProvider.Version, Config: &hcltest.ProviderConfig{ Original: testProvider.Config, VariableCache: variableCaches.GetCache(run.Name, config), @@ -114,7 +113,6 @@ func TransformConfigForTest(config *configs.Config, run *moduletest.Run, file *m NameRange: provider.NameRange, Alias: provider.Alias, AliasRange: provider.AliasRange, - Version: provider.Version, Config: &hcltest.ProviderConfig{ Original: provider.Config, VariableCache: variableCaches.GetCache(run.Name, config),