Merge pull request #1855 from hashicorp/b-module-cycle

Only depend on variables for counts for destroy nodes

terraform/context_test.go

@@ -1155,6 +1155,58 @@ func TestContext2Plan_moduleDestroy(t *testing.T) {
 	}
 }
 
+// GH-1835
+func TestContext2Plan_moduleDestroyCycle(t *testing.T) {
+	m := testModule(t, "plan-module-destroy-gh-1835")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+	s := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: []string{"root", "a_module"},
+				Resources: map[string]*ResourceState{
+					"aws_instance.a": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "a",
+						},
+					},
+				},
+			},
+			&ModuleState{
+				Path: []string{"root", "b_module"},
+				Resources: map[string]*ResourceState{
+					"aws_instance.b": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "b",
+						},
+					},
+				},
+			},
+		},
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State:   s,
+		Destroy: true,
+	})
+
+	plan, err := ctx.Plan()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(plan.String())
+	expected := strings.TrimSpace(testTerraformPlanModuleDestroyCycleStr)
+	if actual != expected {
+		t.Fatalf("bad:\n%s", actual)
+	}
+}
+
 func TestContext2Plan_moduleDestroyMultivar(t *testing.T) {
 	m := testModule(t, "plan-module-destroy-multivar")
 	p := testProvider("aws")

terraform/diff.go

@@ -80,7 +80,18 @@ func (d *Diff) Empty() bool {
 
 func (d *Diff) String() string {
 	var buf bytes.Buffer
+
+	keys := make([]string, 0, len(d.Modules))
+	lookup := make(map[string]*ModuleDiff)
 	for _, m := range d.Modules {
+		key := fmt.Sprintf("module.%s", strings.Join(m.Path[1:], "."))
+		keys = append(keys, key)
+		lookup[key] = m
+	}
+	sort.Strings(keys)
+
+	for _, key := range keys {
+		m := lookup[key]
 		mStr := m.String()
 
 		// If we're the root module, we just write the output directly.
@@ -89,7 +100,7 @@ func (d *Diff) String() string {
 			continue
 		}
 
-		buf.WriteString(fmt.Sprintf("module.%s:\n", strings.Join(m.Path[1:], ".")))
+		buf.WriteString(fmt.Sprintf("%s:\n", key))
 
 		s := bufio.NewScanner(strings.NewReader(mStr))
 		for s.Scan() {

terraform/graph_builder.go

@@ -153,7 +153,7 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer {
 	if len(path) <= 1 {
 		steps = append(steps,
 			// Create the destruction nodes
-			&DestroyTransformer{},
+			&DestroyTransformer{FullDestroy: b.Destroy},
 			&CreateBeforeDestroyTransformer{},
 			b.conditional(&conditionalOpts{
 				If:   func() bool { return !b.Verbose },

terraform/graph_config_node_output.go

@@ -62,7 +62,7 @@ func (n *GraphNodeConfigOutput) Proxy() bool {
 }
 
 // GraphNodeDestroyEdgeInclude impl.
-func (n *GraphNodeConfigOutput) DestroyEdgeInclude() bool {
+func (n *GraphNodeConfigOutput) DestroyEdgeInclude(dag.Vertex) bool {
 	return false
 }
 

terraform/graph_config_node_resource.go

@@ -9,6 +9,12 @@ import (
 	"github.com/hashicorp/terraform/dot"
 )
 
+// GraphNodeCountDependent is implemented by resources for giving only
+// the dependencies they have from the "count" field.
+type GraphNodeCountDependent interface {
+	CountDependentOn() []string
+}
+
 // GraphNodeConfigResource represents a resource within the config graph.
 type GraphNodeConfigResource struct {
 	Resource *config.Resource
@@ -31,6 +37,18 @@ func (n *GraphNodeConfigResource) DependableName() []string {
 	return []string{n.Resource.Id()}
 }
 
+// GraphNodeCountDependent impl.
+func (n *GraphNodeConfigResource) CountDependentOn() []string {
+	result := make([]string, 0, len(n.Resource.RawCount.Variables))
+	for _, v := range n.Resource.RawCount.Variables {
+		if vn := varNameForVar(v); vn != "" {
+			result = append(result, vn)
+		}
+	}
+
+	return result
+}
+
 // GraphNodeDependent impl.
 func (n *GraphNodeConfigResource) DependentOn() []string {
 	result := make([]string, len(n.Resource.DependsOn),

terraform/graph_config_node_variable.go

@@ -55,6 +55,26 @@ func (n *GraphNodeConfigVariable) VariableName() string {
 	return n.Variable.Name
 }
 
+// GraphNodeDestroyEdgeInclude impl.
+func (n *GraphNodeConfigVariable) DestroyEdgeInclude(v dag.Vertex) bool {
+	// Only include this variable in a destroy edge if the source vertex
+	// "v" has a count dependency on this variable.
+	cv, ok := v.(GraphNodeCountDependent)
+	if !ok {
+		return false
+	}
+
+	for _, d := range cv.CountDependentOn() {
+		for _, d2 := range n.DependableName() {
+			if d == d2 {
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
 // GraphNodeProxy impl.
 func (n *GraphNodeConfigVariable) Proxy() bool {
 	return true

terraform/terraform_test.go

@@ -996,6 +996,26 @@ module.child:
     ID = bar
 `
 
+const testTerraformPlanModuleDestroyCycleStr = `
+DIFF:
+
+module.a_module:
+  DESTROY MODULE
+  DESTROY: aws_instance.a
+module.b_module:
+  DESTROY MODULE
+  DESTROY: aws_instance.b
+
+STATE:
+
+module.a_module:
+  aws_instance.a:
+    ID = a
+module.b_module:
+  aws_instance.b:
+    ID = b
+`
+
 const testTerraformPlanModuleDestroyMultivarStr = `
 DIFF:
 

terraform/test-fixtures/plan-module-destroy-gh-1835/a/main.tf

@@ -0,0 +1,5 @@
+resource "aws_instance" "a" {}
+
+output "a_output" {
+  value = "${aws_instance.a.id}"
+}

terraform/test-fixtures/plan-module-destroy-gh-1835/b/main.tf

@@ -0,0 +1,5 @@
+variable "a_id" {}
+
+resource "aws_instance" "b" {
+  command = "echo ${var.a_id}"
+}

terraform/test-fixtures/plan-module-destroy-gh-1835/main.tf

@@ -0,0 +1,8 @@
+module "a_module" {
+  source = "./a"
+}
+
+module "b_module" {
+  source = "./b"
+  a_id = "${module.a_module.a_output}"
+}

terraform/transform_destroy.go

@@ -49,12 +49,14 @@ type GraphNodeDestroyPrunable interface {
 // as an edge within the destroy graph. This is usually done because it
 // might cause unnecessary cycles.
 type GraphNodeDestroyEdgeInclude interface {
-	DestroyEdgeInclude() bool
+	DestroyEdgeInclude(dag.Vertex) bool
 }
 
 // DestroyTransformer is a GraphTransformer that creates the destruction
 // nodes for things that _might_ be destroyed.
-type DestroyTransformer struct{}
+type DestroyTransformer struct {
+	FullDestroy bool
+}
 
 func (t *DestroyTransformer) Transform(g *Graph) error {
 	var connect, remove []dag.Edge
@@ -111,7 +113,8 @@ func (t *DestroyTransformer) transform(
 		for _, edgeRaw := range downEdges {
 			// If this thing specifically requests to not be depended on
 			// by destroy nodes, then don't.
-			if i, ok := edgeRaw.(GraphNodeDestroyEdgeInclude); ok && !i.DestroyEdgeInclude() {
+			if i, ok := edgeRaw.(GraphNodeDestroyEdgeInclude); ok &&
+				!i.DestroyEdgeInclude(v) {
 				continue
 			}