From 2ac6c127bc17b50fd1a9c5fde6ab5ddaf66d9069 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Fri, 26 Sep 2014 10:03:10 -0700
Subject: [PATCH] terraform: orphans should properly depend on modules

---
 terraform/graph.go      | 90 ++++++++++++++++++++++++-----------------
 terraform/graph_test.go | 56 +++++++++++++++++++++++++
 2 files changed, 109 insertions(+), 37 deletions(-)

diff --git a/terraform/graph.go b/terraform/graph.go
index 7342c88fb..dba16975b 100644
--- a/terraform/graph.go
+++ b/terraform/graph.go
@@ -236,6 +236,9 @@ func Graph(opts *GraphOpts) (*depgraph.Graph, error) {
 		}
 	}
 
+	// Add the orphan dependencies
+	graphAddOrphanDeps(g, modState)
+
 	// Add the provider dependencies
 	graphAddResourceProviderDeps(g)
 
@@ -771,6 +774,56 @@ func graphAddModuleOrphans(
 	return nil
 }
 
+// graphAddOrphanDeps adds the dependencies to the orphans based on their
+// explicit Dependencies state.
+func graphAddOrphanDeps(g *depgraph.Graph, mod *ModuleState) {
+	for _, n := range g.Nouns {
+		rn, ok := n.Meta.(*GraphNodeResource)
+		if !ok {
+			continue
+		}
+		if rn.Resource.Flags&FlagOrphan == 0 {
+			continue
+		}
+
+		// If we have no dependencies, then just continue
+		rs := mod.Resources[n.Name]
+		if len(rs.Dependencies) == 0 {
+			continue
+		}
+
+		for _, n2 := range g.Nouns {
+			// Don't ever depend on ourselves
+			if n2.Meta == n.Meta {
+				continue
+			}
+
+			var compareName string
+			switch rn2 := n2.Meta.(type) {
+			case *GraphNodeModule:
+				compareName = n2.Name
+			case *GraphNodeResource:
+				compareName = rn2.Resource.Id
+			}
+			if compareName == "" {
+				continue
+			}
+
+			for _, depName := range rs.Dependencies {
+				if compareName != depName {
+					continue
+				}
+				dep := &depgraph.Dependency{
+					Name:   depName,
+					Source: n,
+					Target: n2,
+				}
+				n.Deps = append(n.Deps, dep)
+			}
+		}
+	}
+}
+
 // graphAddOrphans adds the orphans to the graph.
 func graphAddOrphans(g *depgraph.Graph, c *config.Config, mod *ModuleState) {
 	meta := g.Meta.(*GraphMeta)
@@ -802,43 +855,6 @@ func graphAddOrphans(g *depgraph.Graph, c *config.Config, mod *ModuleState) {
 
 	// Add the nouns to the graph
 	g.Nouns = append(g.Nouns, nlist...)
-
-	// Handle the orphan dependencies after adding them
-	// to the graph because there may be depedencies between the
-	// orphans that otherwise cannot be handled
-	for _, n := range nlist {
-		rn := n.Meta.(*GraphNodeResource)
-
-		// If we have no dependencies, then just continue
-		rs := mod.Resources[n.Name]
-		if len(rs.Dependencies) == 0 {
-			continue
-		}
-
-		for _, n2 := range g.Nouns {
-			rn2, ok := n2.Meta.(*GraphNodeResource)
-			if !ok {
-				continue
-			}
-
-			// Don't ever depend on ourselves
-			if rn2 == rn {
-				continue
-			}
-
-			for _, depName := range rs.Dependencies {
-				if rn2.Resource.Id != depName {
-					continue
-				}
-				dep := &depgraph.Dependency{
-					Name:   depName,
-					Source: n,
-					Target: n2,
-				}
-				n.Deps = append(n.Deps, dep)
-			}
-		}
-	}
 }
 
 // graphAddParentProviderConfigs goes through and adds/merges provider
diff --git a/terraform/graph_test.go b/terraform/graph_test.go
index bfb1167f6..858ad1eda 100644
--- a/terraform/graph_test.go
+++ b/terraform/graph_test.go
@@ -824,6 +824,40 @@ func TestGraph_orphan_dependencies(t *testing.T) {
 	}
 }
 
+func TestGraph_orphanDependenciesModules(t *testing.T) {
+	m := testModule(t, "graph-modules")
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+
+				Resources: map[string]*ResourceState{
+					"aws_instance.foo": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "foo",
+						},
+						Dependencies: []string{
+							"module.consul",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	g, err := Graph(&GraphOpts{Module: m, State: state})
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTerraformGraphOrphanModuleDepsStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\nactual:\n%s\n\nexpected:\n%s", actual, expected)
+	}
+}
+
 const testTerraformGraphStr = `
 root: root
 aws_instance.web
@@ -1094,3 +1128,25 @@ root
   root -> aws_load_balancer.old
   root -> aws_load_balancer.weblb
 `
+
+const testTerraformGraphOrphanModuleDepsStr = `
+root: root
+aws_instance.foo
+  aws_instance.foo -> module.consul
+  aws_instance.foo -> provider.aws
+aws_instance.web
+  aws_instance.web -> aws_security_group.firewall
+  aws_instance.web -> module.consul
+  aws_instance.web -> provider.aws
+aws_security_group.firewall
+  aws_security_group.firewall -> provider.aws
+module.consul
+  module.consul -> aws_security_group.firewall
+  module.consul -> provider.aws
+provider.aws
+root
+  root -> aws_instance.foo
+  root -> aws_instance.web
+  root -> aws_security_group.firewall
+  root -> module.consul
+`