From cf3a259cd9fd78bb9f2bc0949866b0708a48c10e Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 3 Nov 2016 15:43:34 -0400
Subject: [PATCH] fix CreateBeforeDestroy with datasources

The graph transformation we implement around create_before_destroy
need to re-order all resources that depend on the create_before_destroy
resource. Up until now, we've requires that users mark all of these
resources as create_before_destroy. Data soruces however don't have a
lifecycle block for create_before_destroy, and could not be marked this
way.

This PR checks each DestroyNode that doesn't implement CreateBeforeDestroy
for any ancestors that do implement CreateBeforeDestroy. If there are
any, we inherit the behavior and re-order the graph as such.
---
 terraform/graph_builder_test.go |  5 +++--
 terraform/transform_destroy.go  | 30 ++++++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/terraform/graph_builder_test.go b/terraform/graph_builder_test.go
index 2cb76757b..f73308b2c 100644
--- a/terraform/graph_builder_test.go
+++ b/terraform/graph_builder_test.go
@@ -173,6 +173,7 @@ func TestBuiltinGraphBuilder_cbdDepNonCbd(t *testing.T) {
 	}
 }
 
+// This now returns no errors due to a general fix while building the graph
 func TestBuiltinGraphBuilder_cbdDepNonCbd_errorsWhenVerbose(t *testing.T) {
 	b := &BuiltinGraphBuilder{
 		Root:     testModule(t, "graph-builder-cbd-non-cbd"),
@@ -181,8 +182,8 @@ func TestBuiltinGraphBuilder_cbdDepNonCbd_errorsWhenVerbose(t *testing.T) {
 	}
 
 	_, err := b.Build(RootModulePath)
-	if err == nil {
-		t.Fatalf("expected err, got none")
+	if err != nil {
+		t.Fatalf("err: %s", err)
 	}
 }
 
diff --git a/terraform/transform_destroy.go b/terraform/transform_destroy.go
index af8ccc4ab..47c42c0da 100644
--- a/terraform/transform_destroy.go
+++ b/terraform/transform_destroy.go
@@ -1,8 +1,6 @@
 package terraform
 
-import (
-	"github.com/hashicorp/terraform/dag"
-)
+import "github.com/hashicorp/terraform/dag"
 
 // GraphNodeDestroyable is the interface that nodes that can be destroyed
 // must implement. This is used to automatically handle the creation of
@@ -153,7 +151,7 @@ func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error {
 		}
 
 		// If the node doesn't need to create before destroy, then continue
-		if !dn.CreateBeforeDestroy() {
+		if !dn.CreateBeforeDestroy() && noCreateBeforeDestroyAncestors(g, dn) {
 			continue
 		}
 
@@ -200,6 +198,30 @@ func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error {
 	return nil
 }
 
+// noCreateBeforeDestroyAncestors verifies that a vertex has no ancestors that
+// are CreateBeforeDestroy.
+// If this vertex has an ancestor with CreateBeforeDestroy, we will need to
+// inherit that behavior and re-order the edges even if this node type doesn't
+// directly implement CreateBeforeDestroy.
+func noCreateBeforeDestroyAncestors(g *Graph, v dag.Vertex) bool {
+	s, _ := g.Ancestors(v)
+	if s == nil {
+		return true
+	}
+	for _, v := range s.List() {
+		dn, ok := v.(GraphNodeDestroy)
+		if !ok {
+			continue
+		}
+
+		if dn.CreateBeforeDestroy() {
+			// some ancestor is CreateBeforeDestroy, so we need to follow suit
+			return false
+		}
+	}
+	return true
+}
+
 // PruneDestroyTransformer is a GraphTransformer that removes the destroy
 // nodes that aren't in the diff.
 type PruneDestroyTransformer struct {