Merge pull request #6599 from hashicorp/phinze/destroy-interpolation-error-redux

terraform: Correct fix for destroy interp errors

terraform/context_apply_test.go

@@ -2549,19 +2549,16 @@ func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) {
 			Providers: map[string]ResourceProviderFactory{
 				"aws": testProviderFuncFixed(p),
 			},
-			Variables: map[string]string{
-				"key_name": "foobarkey",
-			},
 		})
 
 		// First plan and apply a create operation
 		if _, err := ctx.Plan(); err != nil {
-			t.Fatalf("err: %s", err)
+			t.Fatalf("plan err: %s", err)
 		}
 
 		state, err = ctx.Apply()
 		if err != nil {
-			t.Fatalf("err: %s", err)
+			t.Fatalf("apply err: %s", err)
 		}
 	}
 
@@ -2585,17 +2582,17 @@ func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) {
 		// First plan and apply a create operation
 		plan, err := ctx.Plan()
 		if err != nil {
-			t.Fatalf("err: %s", err)
+			t.Fatalf("destroy plan err: %s", err)
 		}
 
 		var buf bytes.Buffer
 		if err := WritePlan(plan, &buf); err != nil {
-			t.Fatalf("err: %s", err)
+			t.Fatalf("plan write err: %s", err)
 		}
 
 		planFromFile, err := ReadPlan(&buf)
 		if err != nil {
-			t.Fatalf("err: %s", err)
+			t.Fatalf("plan read err: %s", err)
 		}
 
 		ctx, err = planFromFile.Context(&ContextOpts{
@@ -2609,7 +2606,7 @@ func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) {
 
 		state, err = ctx.Apply()
 		if err != nil {
-			t.Fatalf("err: %s", err)
+			t.Fatalf("destroy apply err: %s", err)
 		}
 	}
 

terraform/graph_config_node_resource.go

@@ -2,6 +2,7 @@ package terraform
 
 import (
 	"fmt"
+	"log"
 	"strings"
 
 	"github.com/hashicorp/terraform/config"
@@ -272,14 +273,17 @@ func (n *GraphNodeConfigResource) DestroyNode(mode GraphNodeDestroyMode) GraphNo
 
 // GraphNodeNoopPrunable
 func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool {
+	log.Printf("[DEBUG] Checking resource noop: %s", n.Name())
 	// We don't have any noop optimizations for destroy nodes yet
 	if n.DestroyMode != DestroyNone {
+		log.Printf("[DEBUG] Destroy node, not a noop")
 		return false
 	}
 
 	// If there is no diff, then we aren't a noop since something needs to
 	// be done (such as a plan). We only check if we're a noop in a diff.
 	if opts.Diff == nil || opts.Diff.Empty() {
+		log.Printf("[DEBUG] No diff, not a noop")
 		return false
 	}
 
@@ -287,6 +291,7 @@ func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool {
 	// we need to be sure to evaluate the count so that splat variables work
 	// later (which need to know the full count).
 	if len(n.Resource.RawCount.Interpolations) > 0 {
+		log.Printf("[DEBUG] Count has interpolations, not a noop")
 		return false
 	}
 
@@ -294,12 +299,7 @@ func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool {
 	// it means there is a diff, and that the module we're in just isn't
 	// in it, meaning we're not doing anything.
 	if opts.ModDiff == nil || opts.ModDiff.Empty() {
-		return true
+		log.Printf("[DEBUG] No mod diff, treating resource as a noop")
-	}
-
-	// If the whole module is being destroyed, then the resource nodes in that
-	// module are irrelevant - we only need to keep the destroy nodes.
-	if opts.ModDiff != nil && opts.ModDiff.Destroy == true {
 		return true
 	}
 
@@ -310,11 +310,13 @@ func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool {
 	found := false
 	for k, _ := range opts.ModDiff.Resources {
 		if strings.HasPrefix(k, prefix) {
+			log.Printf("[DEBUG] Diff has %s, resource is not a noop", k)
 			found = true
 			break
 		}
 	}
 
+	log.Printf("[DEBUG] Final noop value: %t", !found)
 	return !found
 }
 

terraform/graph_config_node_variable.go

@@ -2,6 +2,7 @@ package terraform
 
 import (
 	"fmt"
+	"log"
 
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/config/module"
@@ -79,23 +80,37 @@ func (n *GraphNodeConfigVariable) DestroyEdgeInclude(v dag.Vertex) bool {
 
 // GraphNodeNoopPrunable
 func (n *GraphNodeConfigVariable) Noop(opts *NoopOpts) bool {
+	log.Printf("[DEBUG] Checking variable noop: %s", n.Name())
 	// If we have no diff, always keep this in the graph. We have to do
 	// this primarily for validation: we want to validate that variable
 	// interpolations are valid even if there are no resources that
 	// depend on them.
 	if opts.Diff == nil || opts.Diff.Empty() {
+		log.Printf("[DEBUG] No diff, not a noop")
 		return false
 	}
 
+	// We have to find our our module diff since we do funky things with
+	// the flat node's implementation of Path() below.
+	modDiff := opts.Diff.ModuleByPath(n.ModulePath)
+
+	// If we're destroying, we have no need of variables.
+	if modDiff != nil && modDiff.Destroy {
+		log.Printf("[DEBUG] Destroy diff, treating variable as a noop")
+		return true
+	}
+
 	for _, v := range opts.Graph.UpEdges(opts.Vertex).List() {
 		// This is terrible, but I can't think of a better way to do this.
 		if dag.VertexName(v) == rootNodeName {
 			continue
 		}
 
+		log.Printf("[DEBUG] Found up edge to %s, var is not noop", dag.VertexName(v))
 		return false
 	}
 
+	log.Printf("[DEBUG] No up edges, treating variable as a noop")
 	return true
 }
 

terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf

@@ -3,3 +3,7 @@ variable "vpc_id" {}
 resource "aws_instance" "child" {
   vpc_id = "${var.vpc_id}"
 }
+
+output "modout" {
+  value = "${aws_instance.child.id}"
+}

terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf

@@ -4,3 +4,7 @@ module "child" {
   source = "./child"
   vpc_id = "${aws_instance.vpc.id}"
 }
+
+output "out" {
+  value = "${module.child.modout}"
+}