From 7407fee9c20deab88e551791e44ed073c1281b10 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Wed, 4 Mar 2020 17:38:39 -0500 Subject: [PATCH 1/5] Make modules targetable --- addrs/module.go | 50 ++++++++++++++++++++++++++++++++++++++++ addrs/module_instance.go | 17 +++++++++++--- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/addrs/module.go b/addrs/module.go index c887718b1..fb41ded61 100644 --- a/addrs/module.go +++ b/addrs/module.go @@ -44,6 +44,56 @@ func (m Module) Equal(other Module) bool { return m.String() == other.String() } +func (m Module) targetableSigil() { + // Module is targetable +} + +// TargetContains implements Targetable for Module by returning true if the given other +// address either matches the receiver, is a sub-module-instance of the +// receiver, or is a targetable absolute address within a module that +// is contained within the receiver. +func (m Module) TargetContains(other Targetable) bool { + switch to := other.(type) { + + case Module: + if len(to) < len(m) { + // Can't be contained if the path is shorter + return false + } + // Other is contained if its steps match for the length of our own path. + for i, ourStep := range m { + otherStep := to[i] + if ourStep != otherStep { + return false + } + } + // If we fall out here then the prefixed matched, so it's contained. + return true + + case ModuleInstance: + if len(to) < len(m) { + return false + } + for i, ourStep := range m { + otherStep := to[i] + // This is where ModuleInstance differs from Module + if ourStep != otherStep.Name { + return false + } + } + return true + + case AbsResource: + return m.TargetContains(to.Module) + + case AbsResourceInstance: + return m.TargetContains(to.Module) + + default: + return false + } +} + // Child returns the address of a child call in the receiver, identified by the // given name. func (m Module) Child(name string) Module { diff --git a/addrs/module_instance.go b/addrs/module_instance.go index b2b51717a..9647e5e5c 100644 --- a/addrs/module_instance.go +++ b/addrs/module_instance.go @@ -383,8 +383,7 @@ func (m ModuleInstance) CallInstance() (ModuleInstance, ModuleCallInstance) { // is contained within the reciever. func (m ModuleInstance) TargetContains(other Targetable) bool { switch to := other.(type) { - - case ModuleInstance: + case Module: if len(to) < len(m) { // Can't be contained if the path is shorter return false @@ -392,13 +391,25 @@ func (m ModuleInstance) TargetContains(other Targetable) bool { // Other is contained if its steps match for the length of our own path. for i, ourStep := range m { otherStep := to[i] - if ourStep != otherStep { + if ourStep.Name != otherStep { return false } } // If we fall out here then the prefixed matched, so it's contained. return true + case ModuleInstance: + if len(to) < len(m) { + return false + } + for i, ourStep := range m { + otherStep := to[i] + if ourStep != otherStep { + return false + } + } + return true + case AbsResource: return m.TargetContains(to.Module) From 076c5400768edf249e2c78a4930bc8bb889f6587 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 6 Mar 2020 16:07:57 -0500 Subject: [PATCH 2/5] Add a test for whole module targeting --- terraform/transform_targets_test.go | 71 +++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/terraform/transform_targets_test.go b/terraform/transform_targets_test.go index f29109af4..02ea58d46 100644 --- a/terraform/transform_targets_test.go +++ b/terraform/transform_targets_test.go @@ -130,6 +130,77 @@ output.grandchild_id } } +// This tests the TargetsTransformer targeting a whole module, +// rather than a resource within a module instance. +func TestTargetsTransformer_wholeModule(t *testing.T) { + mod := testModule(t, "transform-targets-downstream") + + g := Graph{Path: addrs.RootModuleInstance} + { + transform := &ConfigTransformer{Config: mod} + if err := transform.Transform(&g); err != nil { + t.Fatalf("%T failed: %s", transform, err) + } + } + + { + transform := &AttachResourceConfigTransformer{Config: mod} + if err := transform.Transform(&g); err != nil { + t.Fatalf("%T failed: %s", transform, err) + } + } + + { + transform := &AttachResourceConfigTransformer{Config: mod} + if err := transform.Transform(&g); err != nil { + t.Fatalf("%T failed: %s", transform, err) + } + } + + { + transform := &OutputTransformer{Config: mod} + if err := transform.Transform(&g); err != nil { + t.Fatalf("%T failed: %s", transform, err) + } + } + + { + transform := &ReferenceTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + transform := &TargetsTransformer{ + Targets: []addrs.Targetable{ + addrs.RootModule. + Child("child"). + Child("grandchild"), + }, + } + if err := transform.Transform(&g); err != nil { + t.Fatalf("%T failed: %s", transform, err) + } + } + + actual := strings.TrimSpace(g.String()) + // Even though we only asked to target the grandchild module, all of the + // outputs that descend from it are also targeted. + expected := strings.TrimSpace(` +module.child.module.grandchild.aws_instance.foo +module.child.module.grandchild.output.id + module.child.module.grandchild.aws_instance.foo +module.child.output.grandchild_id + module.child.module.grandchild.output.id +output.grandchild_id + module.child.output.grandchild_id + `) + if actual != expected { + t.Fatalf("bad:\n\nexpected:\n%s\n\ngot:\n%s\n", expected, actual) + } +} + func TestTargetsTransformer_destroy(t *testing.T) { mod := testModule(t, "transform-targets-destroy") From bf91bff2c896180b4e873576aa0d9ecd276feb2e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 12 Mar 2020 18:14:44 -0400 Subject: [PATCH 3/5] TargetContains improvements Simplify Module.TargetContains Handle the case of a keyed instance address in ModuleInstance.TargetContains. --- addrs/module.go | 12 +----------- addrs/module_instance.go | 10 ++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/addrs/module.go b/addrs/module.go index fb41ded61..62271b298 100644 --- a/addrs/module.go +++ b/addrs/module.go @@ -71,17 +71,7 @@ func (m Module) TargetContains(other Targetable) bool { return true case ModuleInstance: - if len(to) < len(m) { - return false - } - for i, ourStep := range m { - otherStep := to[i] - // This is where ModuleInstance differs from Module - if ourStep != otherStep.Name { - return false - } - } - return true + return m.TargetContains(to.Module()) case AbsResource: return m.TargetContains(to.Module) diff --git a/addrs/module_instance.go b/addrs/module_instance.go index 9647e5e5c..40f765ebc 100644 --- a/addrs/module_instance.go +++ b/addrs/module_instance.go @@ -391,6 +391,16 @@ func (m ModuleInstance) TargetContains(other Targetable) bool { // Other is contained if its steps match for the length of our own path. for i, ourStep := range m { otherStep := to[i] + + // We can't contain an entire module if we have a specific instance + // key. The case of NoKey is OK because this address is either + // meant to address an unexpanded module, or a single instance of + // that module, and both of those are a covered in-full by the + // Module address. + if ourStep.InstanceKey != NoKey { + return false + } + if ourStep.Name != otherStep { return false } From e3ad9ffb779315d178aa18d1aba900427b952005 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 13 Mar 2020 09:19:27 -0400 Subject: [PATCH 4/5] added module targetting tests --- addrs/target_test.go | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/addrs/target_test.go b/addrs/target_test.go index a16111560..8d5c9cc9c 100644 --- a/addrs/target_test.go +++ b/addrs/target_test.go @@ -95,7 +95,8 @@ func TestTargetContains(t *testing.T) { false, }, - // Config paths, while never returned from parsing a target, must still be targetable + // Config paths, while never returned from parsing a target, must still + // be targetable { ConfigResource{ Module: []string{"bar"}, @@ -131,6 +132,45 @@ func TestTargetContains(t *testing.T) { mustParseTarget("module.bar[0].test_resource.foo"), true, }, + + // Modules are also never the result of parsing a target, but also need + // to be targetable + { + Module{"bar"}, + Module{"bar", "baz"}, + true, + }, + { + Module{"bar"}, + mustParseTarget("module.bar[0]"), + true, + }, + { + // Parsing an ambiguous module path needs to ensure the + // ModuleInstance could contain the Module. This is safe because if + // the module could be expanded, it must have an index, meaning no + // index indicates that the module instance and module are + // functionally equivalent. + mustParseTarget("module.bar"), + Module{"bar"}, + true, + }, + { + // A specific ModuleInstance cannot contain a module + mustParseTarget("module.bar[0]"), + Module{"bar"}, + false, + }, + { + Module{"bar", "baz"}, + mustParseTarget("module.bar[0].module.baz.test_resource.foo[1]"), + true, + }, + { + mustParseTarget("module.bar[0].module.baz"), + Module{"bar", "baz"}, + false, + }, } { t.Run(fmt.Sprintf("%s-in-%s", test.other, test.addr), func(t *testing.T) { got := test.addr.TargetContains(test.other) From e6bac359edeba0d1dbbcd91263a03d22343ff636 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 13 Mar 2020 19:01:23 -0400 Subject: [PATCH 5/5] Missing ConfigResource checks in TargetContains Adding some missing checks for ConfigResource, and check all combinations of Resource types for consistency. --- addrs/module.go | 3 +++ addrs/module_instance.go | 3 +++ addrs/resource.go | 11 +++++++++++ addrs/target_test.go | 29 +++++++++++++++++++++++++++++ 4 files changed, 46 insertions(+) diff --git a/addrs/module.go b/addrs/module.go index 62271b298..2a3ffe3fc 100644 --- a/addrs/module.go +++ b/addrs/module.go @@ -73,6 +73,9 @@ func (m Module) TargetContains(other Targetable) bool { case ModuleInstance: return m.TargetContains(to.Module()) + case ConfigResource: + return m.TargetContains(to.Module) + case AbsResource: return m.TargetContains(to.Module) diff --git a/addrs/module_instance.go b/addrs/module_instance.go index 40f765ebc..49cbf7863 100644 --- a/addrs/module_instance.go +++ b/addrs/module_instance.go @@ -420,6 +420,9 @@ func (m ModuleInstance) TargetContains(other Targetable) bool { } return true + case ConfigResource: + return m.TargetContains(to.Module) + case AbsResource: return m.TargetContains(to.Module) diff --git a/addrs/resource.go b/addrs/resource.go index 4bad01d15..56b2e5119 100644 --- a/addrs/resource.go +++ b/addrs/resource.go @@ -145,6 +145,11 @@ func (r AbsResource) TargetContains(other Targetable) bool { // We'll use our stringification as a cheat-ish way to test for equality. return to.String() == r.String() + case ConfigResource: + // if an absolute resource from parsing a target address contains a + // ConfigResource, the string representation will match + return to.String() == r.String() + case AbsResourceInstance: return r.TargetContains(to.ContainingResource()) @@ -203,9 +208,15 @@ func (r AbsResourceInstance) ContainingResource() AbsResource { func (r AbsResourceInstance) TargetContains(other Targetable) bool { switch to := other.(type) { + // while we currently don't start with an AbsResourceInstance as a target + // address, check all resource types for consistency. case AbsResourceInstance: // We'll use our stringification as a cheat-ish way to test for equality. return to.String() == r.String() + case ConfigResource: + return to.String() == r.String() + case AbsResource: + return to.String() == r.String() default: return false diff --git a/addrs/target_test.go b/addrs/target_test.go index 8d5c9cc9c..be54f9427 100644 --- a/addrs/target_test.go +++ b/addrs/target_test.go @@ -94,6 +94,11 @@ func TestTargetContains(t *testing.T) { mustParseTarget("module.bar[0].test_resource.foo[2]"), false, }, + { + mustParseTarget("module.bar.test_resource.foo"), + mustParseTarget("module.bar.test_resource.foo[0]"), + true, + }, // Config paths, while never returned from parsing a target, must still // be targetable @@ -109,6 +114,30 @@ func TestTargetContains(t *testing.T) { mustParseTarget("module.bar.test_resource.foo[2]"), true, }, + { + mustParseTarget("module.bar"), + ConfigResource{ + Module: []string{"bar"}, + Resource: Resource{ + Mode: ManagedResourceMode, + Type: "test_resource", + Name: "foo", + }, + }, + true, + }, + { + mustParseTarget("module.bar.test_resource.foo"), + ConfigResource{ + Module: []string{"bar"}, + Resource: Resource{ + Mode: ManagedResourceMode, + Type: "test_resource", + Name: "foo", + }, + }, + true, + }, { ConfigResource{ Resource: Resource{