From 755cef98b07dd69ae7dd8e939952da21853c7950 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 3 Feb 2017 14:24:27 +0100 Subject: [PATCH] terraform: remove ConnectDependents and related interfaces --- terraform/graph.go | 138 +----------------------------------- terraform/graph_dot_test.go | 98 +++++++++++++------------ terraform/graph_test.go | 65 ----------------- 3 files changed, 52 insertions(+), 249 deletions(-) diff --git a/terraform/graph.go b/terraform/graph.go index b6a54f2dd..48ce6a336 100644 --- a/terraform/graph.go +++ b/terraform/graph.go @@ -5,7 +5,6 @@ import ( "log" "runtime/debug" "strings" - "sync" "github.com/hashicorp/terraform/dag" ) @@ -17,8 +16,7 @@ const RootModuleName = "root" var RootModulePath = []string{RootModuleName} // Graph represents the graph that Terraform uses to represent resources -// and their dependencies. Each graph represents only one module, but it -// can contain further modules, which themselves have their own graph. +// and their dependencies. type Graph struct { // Graph is the actual DAG. This is embedded so you can call the DAG // methods directly. @@ -29,127 +27,16 @@ type Graph struct { // RootModuleName Path []string - // dependableMap is a lookaside table for fast lookups for connecting - // dependencies by their GraphNodeDependable value to avoid O(n^3)-like - // situations and turn them into O(1) with respect to the number of new - // edges. - dependableMap map[string]dag.Vertex - // debugName is a name for reference in the debug output. This is usually // to indicate what topmost builder was, and if this graph is a shadow or // not. debugName string - - once sync.Once } func (g *Graph) DirectedGraph() dag.Grapher { return &g.AcyclicGraph } -// Add is the same as dag.Graph.Add. -func (g *Graph) Add(v dag.Vertex) dag.Vertex { - g.once.Do(g.init) - - // Call upwards to add it to the actual graph - g.Graph.Add(v) - - // If this is a depend-able node, then store the lookaside info - if dv, ok := v.(GraphNodeDependable); ok { - for _, n := range dv.DependableName() { - g.dependableMap[n] = v - } - } - - return v -} - -// Remove is the same as dag.Graph.Remove -func (g *Graph) Remove(v dag.Vertex) dag.Vertex { - g.once.Do(g.init) - - // If this is a depend-able node, then remove the lookaside info - if dv, ok := v.(GraphNodeDependable); ok { - for _, n := range dv.DependableName() { - delete(g.dependableMap, n) - } - } - - // Call upwards to remove it from the actual graph - return g.Graph.Remove(v) -} - -// Replace is the same as dag.Graph.Replace -func (g *Graph) Replace(o, n dag.Vertex) bool { - g.once.Do(g.init) - - // Go through and update our lookaside to point to the new vertex - for k, v := range g.dependableMap { - if v == o { - if _, ok := n.(GraphNodeDependable); ok { - g.dependableMap[k] = n - } else { - delete(g.dependableMap, k) - } - } - } - - return g.Graph.Replace(o, n) -} - -// ConnectDependent connects a GraphNodeDependent to all of its -// GraphNodeDependables. It returns the list of dependents it was -// unable to connect to. -func (g *Graph) ConnectDependent(raw dag.Vertex) []string { - v, ok := raw.(GraphNodeDependent) - if !ok { - return nil - } - - return g.ConnectTo(v, v.DependentOn()) -} - -// ConnectDependents goes through the graph, connecting all the -// GraphNodeDependents to GraphNodeDependables. This is safe to call -// multiple times. -// -// To get details on whether dependencies could be found/made, the more -// specific ConnectDependent should be used. -func (g *Graph) ConnectDependents() { - for _, v := range g.Vertices() { - if dv, ok := v.(GraphNodeDependent); ok { - g.ConnectDependent(dv) - } - } -} - -// ConnectFrom creates an edge by finding the source from a DependableName -// and connecting it to the specific vertex. -func (g *Graph) ConnectFrom(source string, target dag.Vertex) { - g.once.Do(g.init) - - if source := g.dependableMap[source]; source != nil { - g.Connect(dag.BasicEdge(source, target)) - } -} - -// ConnectTo connects a vertex to a raw string of targets that are the -// result of DependableName, and returns the list of targets that are missing. -func (g *Graph) ConnectTo(v dag.Vertex, targets []string) []string { - g.once.Do(g.init) - - var missing []string - for _, t := range targets { - if dest := g.dependableMap[t]; dest != nil { - g.Connect(dag.BasicEdge(v, dest)) - } else { - missing = append(missing, t) - } - } - - return missing -} - // Walk walks the graph with the given walker for callbacks. The graph // will be walked with full parallelism, so the walker should expect // to be called in concurrently. @@ -157,12 +44,6 @@ func (g *Graph) Walk(walker GraphWalker) error { return g.walk(walker) } -func (g *Graph) init() { - if g.dependableMap == nil { - g.dependableMap = make(map[string]dag.Vertex) - } -} - func (g *Graph) walk(walker GraphWalker) error { // The callbacks for enter/exiting a graph ctx := walker.EnterPath(g.Path) @@ -289,20 +170,3 @@ func (g *Graph) walk(walker GraphWalker) error { return g.AcyclicGraph.Walk(walkFn) } - -// GraphNodeDependable is an interface which says that a node can be -// depended on (an edge can be placed between this node and another) according -// to the well-known name returned by DependableName. -// -// DependableName can return multiple names it is known by. -type GraphNodeDependable interface { - DependableName() []string -} - -// GraphNodeDependent is an interface which says that a node depends -// on another GraphNodeDependable by some name. By implementing this -// interface, Graph.ConnectDependents() can be called multiple times -// safely and efficiently. -type GraphNodeDependent interface { - DependentOn() []string -} diff --git a/terraform/graph_dot_test.go b/terraform/graph_dot_test.go index 1fad2376d..c204424d9 100644 --- a/terraform/graph_dot_test.go +++ b/terraform/graph_dot_test.go @@ -33,23 +33,27 @@ digraph { root := &testDrawableOrigin{"root"} g.Add(root) - levelOne := []string{"foo", "bar"} - for _, s := range levelOne { - g.Add(&testDrawable{ - VertexName: s, - DependentOnMock: []string{"root"}, - }) + levelOne := []interface{}{"foo", "bar"} + for i, s := range levelOne { + levelOne[i] = &testDrawable{ + VertexName: s.(string), + } + v := levelOne[i] + + g.Add(v) + g.Connect(dag.BasicEdge(v, root)) } levelTwo := []string{"baz", "qux"} for i, s := range levelTwo { - g.Add(&testDrawable{ - VertexName: s, - DependentOnMock: levelOne[i : i+1], - }) + v := &testDrawable{ + VertexName: s, + } + + g.Add(v) + g.Connect(dag.BasicEdge(v, levelOne[i])) } - g.ConnectDependents() return &g }, Expect: ` @@ -81,22 +85,23 @@ digraph { root := &testDrawableOrigin{"root"} g.Add(root) - g.Add(&testDrawable{ - VertexName: "A", - DependentOnMock: []string{"root", "C"}, + vA := g.Add(&testDrawable{ + VertexName: "A", }) - g.Add(&testDrawable{ - VertexName: "B", - DependentOnMock: []string{"A"}, + vB := g.Add(&testDrawable{ + VertexName: "B", }) - g.Add(&testDrawable{ - VertexName: "C", - DependentOnMock: []string{"B"}, + vC := g.Add(&testDrawable{ + VertexName: "C", }) - g.ConnectDependents() + g.Connect(dag.BasicEdge(vA, root)) + g.Connect(dag.BasicEdge(vA, vC)) + g.Connect(dag.BasicEdge(vB, vA)) + g.Connect(dag.BasicEdge(vC, vB)) + return &g }, Expect: ` @@ -117,7 +122,7 @@ digraph { "[root] C" -> "[root] B" } } - `, + `, }, { @@ -131,23 +136,23 @@ digraph { g.Add(root) var sub Graph - sub.Add(&testDrawableOrigin{"sub_root"}) + vSubRoot := sub.Add(&testDrawableOrigin{"sub_root"}) var subsub Graph subsub.Add(&testDrawableOrigin{"subsub_root"}) - sub.Add(&testDrawableSubgraph{ - VertexName: "subsub", - SubgraphMock: &subsub, - DependentOnMock: []string{"sub_root"}, - }) - g.Add(&testDrawableSubgraph{ - VertexName: "sub", - SubgraphMock: &sub, - DependentOnMock: []string{"root"}, + vSubV := sub.Add(&testDrawableSubgraph{ + VertexName: "subsub", + SubgraphMock: &subsub, }) - g.ConnectDependents() - sub.ConnectDependents() + vSub := g.Add(&testDrawableSubgraph{ + VertexName: "sub", + SubgraphMock: &sub, + }) + + g.Connect(dag.BasicEdge(vSub, root)) + sub.Connect(dag.BasicEdge(vSubV, vSubRoot)) + return &g }, Expect: ` @@ -170,7 +175,7 @@ digraph { "[subsub] subsub_root" } } - `, + `, }, { @@ -184,23 +189,22 @@ digraph { g.Add(root) var sub Graph - sub.Add(&testDrawableOrigin{"sub_root"}) + rootSub := sub.Add(&testDrawableOrigin{"sub_root"}) var subsub Graph subsub.Add(&testDrawableOrigin{"subsub_root"}) - sub.Add(&testDrawableSubgraph{ - VertexName: "subsub", - SubgraphMock: &subsub, - DependentOnMock: []string{"sub_root"}, + + subV := sub.Add(&testDrawableSubgraph{ + VertexName: "subsub", + SubgraphMock: &subsub, }) - g.Add(&testDrawableSubgraph{ - VertexName: "sub", - SubgraphMock: &sub, - DependentOnMock: []string{"root"}, + vSub := g.Add(&testDrawableSubgraph{ + VertexName: "sub", + SubgraphMock: &sub, }) - g.ConnectDependents() - sub.ConnectDependents() + g.Connect(dag.BasicEdge(vSub, root)) + sub.Connect(dag.BasicEdge(subV, rootSub)) return &g }, Expect: ` @@ -219,7 +223,7 @@ digraph { "[sub] subsub" -> "[sub] sub_root" } } - `, + `, }, } diff --git a/terraform/graph_test.go b/terraform/graph_test.go index fc9fc25dd..db4ddc403 100644 --- a/terraform/graph_test.go +++ b/terraform/graph_test.go @@ -1,76 +1,11 @@ package terraform import ( - "reflect" - "strings" "testing" "github.com/hashicorp/terraform/dag" ) -func TestGraphAdd(t *testing.T) { - // Test Add since we override it and want to make sure we don't break it. - var g Graph - g.Add(42) - g.Add(84) - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testGraphAddStr) - if actual != expected { - t.Fatalf("bad: %s", actual) - } -} - -func TestGraphConnectDependent(t *testing.T) { - var g Graph - g.Add(&testGraphDependable{VertexName: "a"}) - b := g.Add(&testGraphDependable{ - VertexName: "b", - DependentOnMock: []string{"a"}, - }) - - if missing := g.ConnectDependent(b); len(missing) > 0 { - t.Fatalf("bad: %#v", missing) - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testGraphConnectDepsStr) - if actual != expected { - t.Fatalf("bad: %s", actual) - } -} - -func TestGraphReplace_DependableWithNonDependable(t *testing.T) { - var g Graph - a := g.Add(&testGraphDependable{VertexName: "a"}) - b := g.Add(&testGraphDependable{ - VertexName: "b", - DependentOnMock: []string{"a"}, - }) - newA := "non-dependable-a" - - if missing := g.ConnectDependent(b); len(missing) > 0 { - t.Fatalf("bad: %#v", missing) - } - - if !g.Replace(a, newA) { - t.Fatalf("failed to replace") - } - - c := g.Add(&testGraphDependable{ - VertexName: "c", - DependentOnMock: []string{"a"}, - }) - - // This should fail by reporting missing, since a node with dependable - // name "a" is no longer in the graph. - missing := g.ConnectDependent(c) - expected := []string{"a"} - if !reflect.DeepEqual(expected, missing) { - t.Fatalf("expected: %#v, got: %#v", expected, missing) - } -} - func TestGraphWalk_panicWrap(t *testing.T) { var g Graph