Skip to content

Commit

Permalink
configs: A test for the regression reported in #31081
Browse files Browse the repository at this point in the history
5417975 addressed a regression in the
logic for catching when the newer module meta-arguments are used in
conjunction with the legacy practice of including provider configurations
inside modules, where the check started incorrectly catching situations
where the legacy nested provider configuration was in the same module
as the child call using one of those meta-arguments.

This is a regression test to catch if a similar bug arises in the future.

Since it's testing validation rules that apply to an entire configuration
tree, it ended up in a rather idiosyncratic location under the "configload"
package, rather than directly in "configs". The "configs" package only
knows how to load one module at a time, so it's harder to write a test
like this in that context. Due to it being further removed from the code
it is testing, I included a test for the correct error too in order to
increase the chance that we'll learn if future changes in the "configs"
package invalidate this regression test.

I've verified that this new test fails without the change made in the
earlier commit.
  • Loading branch information
apparentlymart committed May 20, 2022
1 parent 94f68a2 commit 5df5618
Show file tree
Hide file tree
Showing 7 changed files with 143 additions and 0 deletions.
74 changes: 74 additions & 0 deletions internal/configs/configload/loader_load_test.go
Expand Up @@ -104,3 +104,77 @@ func TestLoaderLoadConfig_loadDiags(t *testing.T) {
t.Fatal("expected config module")
}
}

func TestLoaderLoadConfig_childProviderGrandchildCount(t *testing.T) {
// This test is focused on the specific situation where:
// - A child module contains a nested provider block, which is no longer
// recommended but supported for backward-compatibility.
// - A child of that child does _not_ contain a nested provider block,
// and is called with "count" (would also apply to "for_each" and
// "depends_on").
// It isn't valid to use "count" with a module that _itself_ contains
// a provider configuration, but it _is_ valid for a module with a
// provider configuration to call another module with count. We previously
// botched this rule and so this is a regression test to cover the
// solution to that mistake:
// https://github.com/hashicorp/terraform/issues/31081

// Since this test is based on success rather than failure and it's
// covering a relatively large set of code where only a small part
// contributes to the test, we'll make sure to test both the success and
// failure cases here so that we'll have a better chance of noticing if a
// future change makes this succeed only because we've reorganized the code
// so that the check isn't happening at all anymore.
//
// If the "not okay" subtest fails, you should also be skeptical about
// whether the "okay" subtest is still valid, even if it happens to
// still be passing.
t.Run("okay", func(t *testing.T) {
fixtureDir := filepath.Clean("testdata/child-provider-grandchild-count")
loader, err := NewLoader(&Config{
ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"),
})
if err != nil {
t.Fatalf("unexpected error from NewLoader: %s", err)
}

cfg, diags := loader.LoadConfig(fixtureDir)
assertNoDiagnostics(t, diags)
if cfg == nil {
t.Fatalf("config is nil; want non-nil")
}

var gotPaths []string
cfg.DeepEach(func(c *configs.Config) {
gotPaths = append(gotPaths, strings.Join(c.Path, "."))
})
sort.Strings(gotPaths)
wantPaths := []string{
"", // root module
"child",
"child.grandchild",
}

if !reflect.DeepEqual(gotPaths, wantPaths) {
t.Fatalf("wrong module paths\ngot: %swant %s", spew.Sdump(gotPaths), spew.Sdump(wantPaths))
}
})
t.Run("not okay", func(t *testing.T) {
fixtureDir := filepath.Clean("testdata/child-provider-child-count")
loader, err := NewLoader(&Config{
ModulesDir: filepath.Join(fixtureDir, ".terraform/modules"),
})
if err != nil {
t.Fatalf("unexpected error from NewLoader: %s", err)
}

_, diags := loader.LoadConfig(fixtureDir)
if !diags.HasErrors() {
t.Fatalf("loading succeeded; want an error")
}
if got, want := diags.Error(), "Module is incompatible with count, for_each, and depends_on"; !strings.Contains(got, want) {
t.Errorf("missing expected error\nwant substring: %s\ngot: %s", want, got)
}
})

}
@@ -0,0 +1,19 @@
{
"Modules": [
{
"Key": "",
"Source": "",
"Dir": "."
},
{
"Key": "child",
"Source": "./child",
"Dir": "testdata/child-provider-grandchild-count/child"
},
{
"Key": "child.grandchild",
"Source": "../grandchild",
"Dir": "testdata/child-provider-grandchild-count/grandchild"
}
]
}
@@ -0,0 +1,4 @@
module "child" {
source = "./child"
count = 1
}
@@ -0,0 +1,12 @@
provider "boop" {
blah = true
}

module "grandchild" {
source = "../grandchild"

# grandchild's caller (this file) has a legacy nested provider block, but
# grandchild itself does not and so it's valid to use "count" here even
# though it wouldn't be valid to call "child" (this file) with "count".
count = 2
}
@@ -0,0 +1,19 @@
{
"Modules": [
{
"Key": "",
"Source": "",
"Dir": "."
},
{
"Key": "child",
"Source": "./child",
"Dir": "testdata/child-provider-grandchild-count/child"
},
{
"Key": "child.grandchild",
"Source": "../grandchild",
"Dir": "testdata/child-provider-grandchild-count/grandchild"
}
]
}
@@ -0,0 +1,3 @@
module "child" {
source = "./child"
}
@@ -0,0 +1,12 @@
provider "boop" {
blah = true
}

module "grandchild" {
source = "../grandchild"

# grandchild's caller (this file) has a legacy nested provider block, but
# grandchild itself does not and so it's valid to use "count" here even
# though it wouldn't be valid to call "child" (this file) with "count".
count = 2
}

0 comments on commit 5df5618

Please sign in to comment.