From 45f7da9678df507be0b3a61caa55e795fc7116ad Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Mon, 22 Jun 2020 12:13:35 -0400 Subject: [PATCH] configs: Fix nested provider requirements bug In a recent PR, we changed the provider requirements code to permit per-module requirements gathering, to enhance the provider command output. This had an incorrect implementation of recursive requirements gathering for the normal case, which resulted in only depth-1 modules being inspected. This commit fixes the broken recursion and adds a grandchild module to the unit tests as test coverage. This also demanded fixing the testNestedModuleConfigFromDir helper function to cope with nested modules in test configs. --- configs/config.go | 22 ++++++++++--------- configs/config_test.go | 17 ++++++++++++-- configs/parser_test.go | 12 ++++++++-- .../grandchild/provider-reqs-grandchild.tf | 4 ++++ .../child/provider-reqs-child.tf | 4 ++++ 5 files changed, 45 insertions(+), 14 deletions(-) create mode 100644 configs/testdata/provider-reqs/child/grandchild/provider-reqs-grandchild.tf diff --git a/configs/config.go b/configs/config.go index 0565ad6bd..bd086e25b 100644 --- a/configs/config.go +++ b/configs/config.go @@ -185,12 +185,7 @@ func (c *Config) DescendentForInstance(path addrs.ModuleInstance) *Config { // may be incomplete. func (c *Config) ProviderRequirements() (getproviders.Requirements, hcl.Diagnostics) { reqs := make(getproviders.Requirements) - diags := c.addProviderRequirements(reqs) - - for _, childConfig := range c.Children { - moreDiags := childConfig.addProviderRequirements(reqs) - diags = append(diags, moreDiags...) - } + diags := c.addProviderRequirements(reqs, true) return reqs, diags } @@ -203,7 +198,7 @@ func (c *Config) ProviderRequirements() (getproviders.Requirements, hcl.Diagnost // may be incomplete. func (c *Config) ProviderRequirementsByModule() (*ModuleRequirements, hcl.Diagnostics) { reqs := make(getproviders.Requirements) - diags := c.addProviderRequirements(reqs) + diags := c.addProviderRequirements(reqs, false) children := make(map[string]*ModuleRequirements) for name, child := range c.Children { @@ -225,9 +220,9 @@ func (c *Config) ProviderRequirementsByModule() (*ModuleRequirements, hcl.Diagno // addProviderRequirements is the main part of the ProviderRequirements // implementation, gradually mutating a shared requirements object to -// eventually return. This function only adds requirements for the top-level -// module. -func (c *Config) addProviderRequirements(reqs getproviders.Requirements) hcl.Diagnostics { +// eventually return. If the recurse argument is true, the requirements will +// include all descendant modules; otherwise, only the specified module. +func (c *Config) addProviderRequirements(reqs getproviders.Requirements, recurse bool) hcl.Diagnostics { var diags hcl.Diagnostics // First we'll deal with the requirements directly in _our_ module... @@ -309,6 +304,13 @@ func (c *Config) addProviderRequirements(reqs getproviders.Requirements) hcl.Dia } } + if recurse { + for _, childConfig := range c.Children { + moreDiags := childConfig.addProviderRequirements(reqs, true) + diags = append(diags, moreDiags...) + } + } + return diags } diff --git a/configs/config_test.go b/configs/config_test.go index 6cfca7850..bf2f65e8c 100644 --- a/configs/config_test.go +++ b/configs/config_test.go @@ -130,6 +130,7 @@ func TestConfigProviderRequirements(t *testing.T) { impliedProvider := addrs.NewDefaultProvider("implied") terraformProvider := addrs.NewBuiltInProvider("terraform") configuredProvider := addrs.NewDefaultProvider("configured") + grandchildProvider := addrs.NewDefaultProvider("grandchild") got, diags := cfg.ProviderRequirements() assertNoDiagnostics(t, diags) @@ -142,6 +143,7 @@ func TestConfigProviderRequirements(t *testing.T) { impliedProvider: nil, happycloudProvider: nil, terraformProvider: nil, + grandchildProvider: nil, } if diff := cmp.Diff(want, got); diff != "" { @@ -166,6 +168,7 @@ func TestConfigProviderRequirementsByModule(t *testing.T) { impliedProvider := addrs.NewDefaultProvider("implied") terraformProvider := addrs.NewBuiltInProvider("terraform") configuredProvider := addrs.NewDefaultProvider("configured") + grandchildProvider := addrs.NewDefaultProvider("grandchild") got, diags := cfg.ProviderRequirementsByModule() assertNoDiagnostics(t, diags) @@ -191,7 +194,17 @@ func TestConfigProviderRequirementsByModule(t *testing.T) { nullProvider: getproviders.MustParseVersionConstraints("= 2.0.1"), happycloudProvider: nil, }, - Children: map[string]*ModuleRequirements{}, + Children: map[string]*ModuleRequirements{ + "nested": { + Name: "nested", + SourceAddr: "./grandchild", + SourceDir: "testdata/provider-reqs/child/grandchild", + Requirements: getproviders.Requirements{ + grandchildProvider: nil, + }, + Children: map[string]*ModuleRequirements{}, + }, + }, }, }, } @@ -227,6 +240,6 @@ func TestConfigAddProviderRequirements(t *testing.T) { reqs := getproviders.Requirements{ addrs.NewDefaultProvider("null"): nil, } - diags = cfg.addProviderRequirements(reqs) + diags = cfg.addProviderRequirements(reqs, true) assertNoDiagnostics(t, diags) } diff --git a/configs/parser_test.go b/configs/parser_test.go index 9cc9776dd..5ecd1f231 100644 --- a/configs/parser_test.go +++ b/configs/parser_test.go @@ -92,10 +92,18 @@ func testNestedModuleConfigFromDir(t *testing.T, path string) (*Config, hcl.Diag cfg, diags := BuildConfig(mod, ModuleWalkerFunc( func(req *ModuleRequest) (*Module, *version.Version, hcl.Diagnostics) { // For the sake of this test we're going to just treat our - // SourceAddr as a path relative to our fixture directory. + // SourceAddr as a path relative to the calling module. // A "real" implementation of ModuleWalker should accept the // various different source address syntaxes Terraform supports. - sourcePath := filepath.Join(path, req.SourceAddr) + + // Build a full path by walking up the module tree, prepending each + // source address path until we hit the root + paths := []string{req.SourceAddr} + for config := req.Parent; config != nil && config.Parent != nil; config = config.Parent { + paths = append([]string{config.SourceAddr}, paths...) + } + paths = append([]string{path}, paths...) + sourcePath := filepath.Join(paths...) mod, diags := parser.LoadConfigDir(sourcePath) version, _ := version.NewVersion(fmt.Sprintf("1.0.%d", versionI)) diff --git a/configs/testdata/provider-reqs/child/grandchild/provider-reqs-grandchild.tf b/configs/testdata/provider-reqs/child/grandchild/provider-reqs-grandchild.tf new file mode 100644 index 000000000..5ac9dfab4 --- /dev/null +++ b/configs/testdata/provider-reqs/child/grandchild/provider-reqs-grandchild.tf @@ -0,0 +1,4 @@ +# There is no provider in required_providers called "grandchild", so this +# implicitly declares a dependency on "hashicorp/grandchild". +resource "grandchild_foo" "bar" { +} diff --git a/configs/testdata/provider-reqs/child/provider-reqs-child.tf b/configs/testdata/provider-reqs/child/provider-reqs-child.tf index ff03ded90..4ebb659dd 100644 --- a/configs/testdata/provider-reqs/child/provider-reqs-child.tf +++ b/configs/testdata/provider-reqs/child/provider-reqs-child.tf @@ -9,3 +9,7 @@ terraform { } } } + +module "nested" { + source = "./grandchild" +}