From c4151b7c7c10f801f2b0c184e68637bfa5e149c8 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Fri, 1 Mar 2019 13:59:12 -0800 Subject: [PATCH] command/show: fixing bugs in modulecalls (#20513) * command/show: fixing bugs in modulecalls jsonconfig and jsonplan both had subtle bugs with the logic for marshaling module calls that only showed up when multiple modules were referenced. This PR fixes those bugs and extends the existing tests to include multiple modules. * sort all the things, mostly for tests --- command/jsonconfig/config.go | 64 ++++++----- command/jsonplan/values.go | 11 +- .../show-json/modules/bar/main.tf | 11 ++ .../show-json/modules/foo/main.tf | 2 +- .../test-fixtures/show-json/modules/main.tf | 12 +- .../show-json/modules/output.json | 106 +++++++++++++++--- 6 files changed, 151 insertions(+), 55 deletions(-) create mode 100644 command/test-fixtures/show-json/modules/bar/main.tf diff --git a/command/jsonconfig/config.go b/command/jsonconfig/config.go index 4fc844c8d..ecfa12c5e 100644 --- a/command/jsonconfig/config.go +++ b/command/jsonconfig/config.go @@ -200,6 +200,7 @@ func marshalModule(c *configs.Config, schemas *terraform.Schemas, addr string) ( outputs[v.Name] = o } module.Outputs = outputs + module.ModuleCalls = marshalModuleCalls(c, schemas) if len(c.Module.Variables) > 0 { @@ -227,42 +228,43 @@ func marshalModule(c *configs.Config, schemas *terraform.Schemas, addr string) ( func marshalModuleCalls(c *configs.Config, schemas *terraform.Schemas) map[string]moduleCall { ret := make(map[string]moduleCall) - for _, mc := range c.Module.ModuleCalls { - retMC := moduleCall{ - Source: mc.SourceAddr, - VersionConstraint: mc.Version.Required.String(), - } - cExp := marshalExpression(mc.Count) - if !cExp.Empty() { - retMC.CountExpression = &cExp - } else { - fExp := marshalExpression(mc.ForEach) - if !fExp.Empty() { - retMC.ForEachExpression = &fExp - } - } - // get the called module's variables so we can build up the expressions - childModule := c.Children[mc.Name] - schema := &configschema.Block{} - schema.Attributes = make(map[string]*configschema.Attribute) - for _, variable := range childModule.Module.Variables { - schema.Attributes[variable.Name] = &configschema.Attribute{ - Required: variable.Default == cty.NilVal, - } - } - - retMC.Expressions = marshalExpressions(mc.Config, schema) - - for name, cc := range c.Children { - childModule, _ := marshalModule(cc, schemas, name) - retMC.Module = childModule - } - ret[mc.Name] = retMC + for name, mc := range c.Module.ModuleCalls { + mcConfig := c.Children[name] + ret[name] = marshalModuleCall(mcConfig, mc, schemas) } return ret +} +func marshalModuleCall(c *configs.Config, mc *configs.ModuleCall, schemas *terraform.Schemas) moduleCall { + ret := moduleCall{ + Source: mc.SourceAddr, + VersionConstraint: mc.Version.Required.String(), + } + cExp := marshalExpression(mc.Count) + if !cExp.Empty() { + ret.CountExpression = &cExp + } else { + fExp := marshalExpression(mc.ForEach) + if !fExp.Empty() { + ret.ForEachExpression = &fExp + } + } + + schema := &configschema.Block{} + schema.Attributes = make(map[string]*configschema.Attribute) + for _, variable := range c.Module.Variables { + schema.Attributes[variable.Name] = &configschema.Attribute{ + Required: variable.Default == cty.NilVal, + } + } + + ret.Expressions = marshalExpressions(mc.Config, schema) + module, _ := marshalModule(c, schemas, mc.Name) + ret.Module = module + + return ret } func marshalResources(resources map[string]*configs.Resource, schemas *terraform.Schemas, moduleAddr string) ([]resource, error) { diff --git a/command/jsonplan/values.go b/command/jsonplan/values.go index 3a91399f1..a6e96d1d8 100644 --- a/command/jsonplan/values.go +++ b/command/jsonplan/values.go @@ -99,9 +99,12 @@ func marshalPlannedValues(changes *plans.Changes, schemas *terraform.Schemas) (m // root has no parents. if containingModule != "" { parent := resource.Addr.Module.Parent().String() - if !seenModules[parent] { + // we likely will see multiple resources in one module, so we + // only need to report the "parent" module for each child module + // once. + if !seenModules[containingModule] { moduleMap[parent] = append(moduleMap[parent], resource.Addr.Module) - seenModules[parent] = true + seenModules[containingModule] = true } } } @@ -118,6 +121,10 @@ func marshalPlannedValues(changes *plans.Changes, schemas *terraform.Schemas) (m if err != nil { return ret, err } + sort.Slice(childModules, func(i, j int) bool { + return childModules[i].Address < childModules[j].Address + }) + ret.ChildModules = childModules return ret, nil diff --git a/command/test-fixtures/show-json/modules/bar/main.tf b/command/test-fixtures/show-json/modules/bar/main.tf new file mode 100644 index 000000000..5d1788a0e --- /dev/null +++ b/command/test-fixtures/show-json/modules/bar/main.tf @@ -0,0 +1,11 @@ +variable "test_var" { + default = "bar-var" +} + +output "test" { + value = var.test_var +} + +resource "test_instance" "test" { + ami = var.test_var +} diff --git a/command/test-fixtures/show-json/modules/foo/main.tf b/command/test-fixtures/show-json/modules/foo/main.tf index 94a022160..2694ca85b 100644 --- a/command/test-fixtures/show-json/modules/foo/main.tf +++ b/command/test-fixtures/show-json/modules/foo/main.tf @@ -1,5 +1,5 @@ variable "test_var" { - default = "bar" + default = "foo-var" } resource "test_instance" "test" { ami = var.test_var diff --git a/command/test-fixtures/show-json/modules/main.tf b/command/test-fixtures/show-json/modules/main.tf index 7ed726c60..d1cabad3e 100644 --- a/command/test-fixtures/show-json/modules/main.tf +++ b/command/test-fixtures/show-json/modules/main.tf @@ -1,9 +1,13 @@ -module "module_test" { +module "module_test_foo" { source = "./foo" test_var = "baz" } -output "test" { - value = module.module_test.test - depends_on = [module.module_test] +module "module_test_bar" { + source = "./bar" +} + +output "test" { + value = module.module_test_foo.test + depends_on = [module.module_test_foo] } diff --git a/command/test-fixtures/show-json/modules/output.json b/command/test-fixtures/show-json/modules/output.json index 5c42b69fb..0284a57e0 100644 --- a/command/test-fixtures/show-json/modules/output.json +++ b/command/test-fixtures/show-json/modules/output.json @@ -12,7 +12,23 @@ { "resources": [ { - "address": "module.module_test.test_instance.test[0]", + "address": "module.module_test_bar.test_instance.test", + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_name": "test", + "schema_version": 0, + "values": { + "ami": "bar-var" + } + } + ], + "address": "module.module_test_bar" + }, + { + "resources": [ + { + "address": "module.module_test_foo.test_instance.test[0]", "mode": "managed", "type": "test_instance", "name": "test", @@ -24,7 +40,7 @@ } }, { - "address": "module.module_test.test_instance.test[1]", + "address": "module.module_test_foo.test_instance.test[1]", "mode": "managed", "type": "test_instance", "name": "test", @@ -36,7 +52,7 @@ } }, { - "address": "module.module_test.test_instance.test[2]", + "address": "module.module_test_foo.test_instance.test[2]", "mode": "managed", "type": "test_instance", "name": "test", @@ -48,15 +64,35 @@ } } ], - "address": "module.module_test" + "address": "module.module_test_foo" } ] } }, "resource_changes": [ { - "address": "module.module_test.test_instance.test[0]", - "module_address": "module.module_test", + "address": "module.module_test_bar.test_instance.test", + "module_address": "module.module_test_bar", + "mode": "managed", + "type": "test_instance", + "name": "test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after": { + "ami": "bar-var" + }, + "after_unknown": { + "ami": false, + "id": true + } + } + }, + { + "address": "module.module_test_foo.test_instance.test[0]", + "module_address": "module.module_test_foo", "mode": "managed", "type": "test_instance", "name": "test", @@ -76,8 +112,8 @@ } }, { - "address": "module.module_test.test_instance.test[1]", - "module_address": "module.module_test", + "address": "module.module_test_foo.test_instance.test[1]", + "module_address": "module.module_test_foo", "mode": "managed", "type": "test_instance", "name": "test", @@ -97,8 +133,8 @@ } }, { - "address": "module.module_test.test_instance.test[2]", - "module_address": "module.module_test", + "address": "module.module_test_foo.test_instance.test[2]", + "module_address": "module.module_test_foo", "mode": "managed", "type": "test_instance", "name": "test", @@ -134,16 +170,52 @@ "test": { "expression": { "references": [ - "module.module_test.test" + "module.module_test_foo.test" ] }, "depends_on": [ - "module.module_test" + "module.module_test_foo" ] } }, "module_calls": { - "module_test": { + "module_test_bar": { + "source": "./bar", + "module": { + "outputs": { + "test": { + "expression": { + "references": [ + "var.test_var" + ] + } + } + }, + "resources": [ + { + "address": "test_instance.test", + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_config_key": "module_test_bar:test", + "expressions": { + "ami": { + "references": [ + "var.test_var" + ] + } + }, + "schema_version": 0 + } + ], + "variables": { + "test_var": { + "default": "bar-var" + } + } + } + }, + "module_test_foo": { "source": "./foo", "expressions": { "test_var": { @@ -166,7 +238,7 @@ "mode": "managed", "type": "test_instance", "name": "test", - "provider_config_key": "module_test:test", + "provider_config_key": "module_test_foo:test", "expressions": { "ami": { "references": [ @@ -182,7 +254,7 @@ ], "variables": { "test_var": { - "default": "bar" + "default": "foo-var" } } } @@ -190,8 +262,8 @@ } }, "provider_config": { - "module_test:test": { - "module_address": "module_test", + "module_test_foo:test": { + "module_address": "module_test_foo", "name": "test" } }