Merge pull request #10745 from hashicorp/b-module-destroy-order

terraform: destroy edges should take into account module variables

terraform/graph_builder_apply_test.go

@@ -192,6 +192,46 @@ func TestApplyGraphBuilder_destroyCount(t *testing.T) {
 	}
 }
 
+func TestApplyGraphBuilder_moduleDestroy(t *testing.T) {
+	diff := &Diff{
+		Modules: []*ModuleDiff{
+			&ModuleDiff{
+				Path: []string{"root", "A"},
+				Resources: map[string]*InstanceDiff{
+					"null_resource.foo": &InstanceDiff{
+						Destroy: true,
+					},
+				},
+			},
+
+			&ModuleDiff{
+				Path: []string{"root", "B"},
+				Resources: map[string]*InstanceDiff{
+					"null_resource.foo": &InstanceDiff{
+						Destroy: true,
+					},
+				},
+			},
+		},
+	}
+
+	b := &ApplyGraphBuilder{
+		Module:    testModule(t, "graph-builder-apply-module-destroy"),
+		Diff:      diff,
+		Providers: []string{"null"},
+	}
+
+	g, err := b.Build(RootModulePath)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	testGraphHappensBefore(
+		t, g,
+		"module.B.null_resource.foo (destroy)",
+		"module.A.null_resource.foo (destroy)")
+}
+
 const testApplyGraphBuilderStr = `
 aws_instance.create
   provider.aws

terraform/graph_test.go

@@ -4,6 +4,8 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+
+	"github.com/hashicorp/terraform/dag"
 )
 
 func TestGraphAdd(t *testing.T) {
@@ -86,6 +88,42 @@ func TestGraphWalk_panicWrap(t *testing.T) {
 	}
 }
 
+// testGraphHappensBefore is an assertion helper that tests that node
+// A (dag.VertexName value) happens before node B.
+func testGraphHappensBefore(t *testing.T, g *Graph, A, B string) {
+	// Find the B vertex
+	var vertexB dag.Vertex
+	for _, v := range g.Vertices() {
+		if dag.VertexName(v) == B {
+			vertexB = v
+			break
+		}
+	}
+	if vertexB == nil {
+		t.Fatalf(
+			"Expected %q before %q. Couldn't find %q in:\n\n%s",
+			A, B, B, g.String())
+	}
+
+	// Look at ancestors
+	deps, err := g.Ancestors(vertexB)
+	if err != nil {
+		t.Fatalf("Error: %s in graph:\n\n%s", err, g.String())
+	}
+
+	// Make sure B is in there
+	for _, v := range deps.List() {
+		if dag.VertexName(v) == A {
+			// Success
+			return
+		}
+	}
+
+	t.Fatalf(
+		"Expected %q before %q in:\n\n%s",
+		A, B, g.String())
+}
+
 type testGraphSubPath struct {
 	PathFn func() []string
 }

terraform/test-fixtures/graph-builder-apply-module-destroy/A/main.tf

@@ -0,0 +1,7 @@
+variable "input" {}
+
+resource "null_resource" "foo" {
+    triggers { input = "${var.input}" }
+}
+
+output "output" { value = "${null_resource.foo.id}" }

terraform/test-fixtures/graph-builder-apply-module-destroy/main.tf

@@ -0,0 +1,11 @@
+variable "input" { default = "value" }
+
+module "A" {
+    source = "./A"
+    input  = "${var.input}"
+}
+
+module "B" {
+    source = "./A"
+    input  = "${module.A.output}"
+}

terraform/transform_destroy_edge.go

@@ -131,6 +131,12 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		&ParentProviderTransformer{},
 		&AttachProviderConfigTransformer{Module: t.Module},
 
+		// Add all the variables. We can depend on resources through
+		// variables due to module parameters, and we need to properly
+		// determine that.
+		&RootVariableTransformer{Module: t.Module},
+		&ModuleVariableTransformer{Module: t.Module},
+
 		&ReferenceTransformer{},
 	}