Merge pull request #4877 from svanharmelen/b-provisioner-graphing

core: fix the provisioner graphing

terraform/graph_builder.go

@@ -136,9 +136,6 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer {
 
 		// Make sure all the connections that are proxies are connected through
 		&ProxyTransformer{},
-
-		// Make sure we have a single root
-		&RootTransformer{},
 	}
 
 	// If we're on the root path, then we do a bunch of other stuff.
@@ -149,11 +146,9 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer {
 			// their dependencies.
 			&TargetsTransformer{Targets: b.Targets, Destroy: b.Destroy},
 
-			// Prune the providers and provisioners. This must happen
+			// Prune the providers. This must happen only once because flattened
-			// only once because flattened modules might depend on empty
+			// modules might depend on empty providers.
-			// providers.
 			&PruneProviderTransformer{},
-			&PruneProvisionerTransformer{},
 
 			// Create the destruction nodes
 			&DestroyTransformer{FullDestroy: b.Destroy},
@@ -170,17 +165,15 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer {
 			&CloseProviderTransformer{},
 			&CloseProvisionerTransformer{},
 
-			// Make sure we have a single root after the above changes.
-			// This is the 2nd root transformer. In practice this shouldn't
-			// actually matter as the RootTransformer is idempotent.
-			&RootTransformer{},
-
 			// Perform the transitive reduction to make our graph a bit
 			// more sane if possible (it usually is possible).
 			&TransitiveReductionTransformer{},
 		)
 	}
 
+	// Make sure we have a single root
+	steps = append(steps, &RootTransformer{})
+
 	// Remove nils
 	for i, s := range steps {
 		if s == nil {

terraform/graph_builder_test.go

@@ -265,6 +265,7 @@ provider.aws (close)
 const testBuiltinGraphBuilderMultiLevelStr = `
 module.foo.module.bar.output.value
   module.foo.module.bar.var.bar
+  module.foo.var.foo
 module.foo.module.bar.plan-destroy
 module.foo.module.bar.var.bar
   module.foo.var.foo
@@ -273,7 +274,9 @@ module.foo.var.foo
 root
   module.foo.module.bar.output.value
   module.foo.module.bar.plan-destroy
+  module.foo.module.bar.var.bar
   module.foo.plan-destroy
+  module.foo.var.foo
 `
 
 const testBuiltinGraphBuilderOrphanDepsStr = `

terraform/transform_provisioner.go

@@ -39,16 +39,15 @@ func (t *ProvisionerTransformer) Transform(g *Graph) error {
 	m := provisionerVertexMap(g)
 	for _, v := range g.Vertices() {
 		if pv, ok := v.(GraphNodeProvisionerConsumer); ok {
-			for _, provisionerName := range pv.ProvisionedBy() {
+			for _, p := range pv.ProvisionedBy() {
-				target := m[provisionerName]
+				if m[p] == nil {
-				if target == nil {
 					err = multierror.Append(err, fmt.Errorf(
 						"%s: provisioner %s couldn't be found",
-						dag.VertexName(v), provisionerName))
+						dag.VertexName(v), p))
 					continue
 				}
 
-				g.Connect(dag.BasicEdge(v, target))
+				g.Connect(dag.BasicEdge(v, m[p]))
 			}
 		}
 	}
@@ -56,6 +55,51 @@ func (t *ProvisionerTransformer) Transform(g *Graph) error {
 	return err
 }
 
+// MissingProvisionerTransformer is a GraphTransformer that adds nodes
+// for missing provisioners into the graph.
+type MissingProvisionerTransformer struct {
+	// Provisioners is the list of provisioners we support.
+	Provisioners []string
+}
+
+func (t *MissingProvisionerTransformer) Transform(g *Graph) error {
+	// Create a set of our supported provisioners
+	supported := make(map[string]struct{}, len(t.Provisioners))
+	for _, v := range t.Provisioners {
+		supported[v] = struct{}{}
+	}
+
+	// Get the map of provisioners we already have in our graph
+	m := provisionerVertexMap(g)
+
+	// Go through all the provisioner consumers and make sure we add
+	// that provisioner if it is missing.
+	for _, v := range g.Vertices() {
+		pv, ok := v.(GraphNodeProvisionerConsumer)
+		if !ok {
+			continue
+		}
+
+		for _, p := range pv.ProvisionedBy() {
+			if _, ok := m[p]; ok {
+				// This provisioner already exists as a configure node
+				continue
+			}
+
+			if _, ok := supported[p]; !ok {
+				// If we don't support the provisioner type, skip it.
+				// Validation later will catch this as an error.
+				continue
+			}
+
+			// Add the missing provisioner node to the graph
+			m[p] = g.Add(&graphNodeProvisioner{ProvisionerNameValue: p})
+		}
+	}
+
+	return nil
+}
+
 // CloseProvisionerTransformer is a GraphTransformer that adds nodes to the
 // graph that will close open provisioner connections that aren't needed
 // anymore. A provisioner connection is not needed anymore once all depended
@@ -87,51 +131,6 @@ func (t *CloseProvisionerTransformer) Transform(g *Graph) error {
 	return nil
 }
 
-// MissingProvisionerTransformer is a GraphTransformer that adds nodes
-// for missing provisioners into the graph. Specifically, it creates provisioner
-// configuration nodes for all the provisioners that we support. These are
-// pruned later during an optimization pass.
-type MissingProvisionerTransformer struct {
-	// Provisioners is the list of provisioners we support.
-	Provisioners []string
-}
-
-func (t *MissingProvisionerTransformer) Transform(g *Graph) error {
-	m := provisionerVertexMap(g)
-	for _, p := range t.Provisioners {
-		if _, ok := m[p]; ok {
-			// This provisioner already exists as a configured node
-			continue
-		}
-
-		// Add our own missing provisioner node to the graph
-		g.Add(&graphNodeMissingProvisioner{ProvisionerNameValue: p})
-	}
-
-	return nil
-}
-
-// PruneProvisionerTransformer is a GraphTransformer that prunes all the
-// provisioners that aren't needed from the graph. A provisioner is unneeded if
-// no resource or module is using that provisioner.
-type PruneProvisionerTransformer struct{}
-
-func (t *PruneProvisionerTransformer) Transform(g *Graph) error {
-	for _, v := range g.Vertices() {
-		// We only care about the provisioners
-		if _, ok := v.(GraphNodeProvisioner); !ok {
-			continue
-		}
-
-		// Does anything depend on this? If not, then prune it.
-		if s := g.UpEdges(v); s.Len() == 0 {
-			g.Remove(v)
-		}
-	}
-
-	return nil
-}
-
 func provisionerVertexMap(g *Graph) map[string]dag.Vertex {
 	m := make(map[string]dag.Vertex)
 	for _, v := range g.Vertices() {
@@ -171,49 +170,49 @@ func (n *graphNodeCloseProvisioner) CloseProvisionerName() string {
 	return n.ProvisionerNameValue
 }
 
-type graphNodeMissingProvisioner struct {
+type graphNodeProvisioner struct {
 	ProvisionerNameValue string
 }
 
-func (n *graphNodeMissingProvisioner) Name() string {
+func (n *graphNodeProvisioner) Name() string {
 	return fmt.Sprintf("provisioner.%s", n.ProvisionerNameValue)
 }
 
 // GraphNodeEvalable impl.
-func (n *graphNodeMissingProvisioner) EvalTree() EvalNode {
+func (n *graphNodeProvisioner) EvalTree() EvalNode {
 	return &EvalInitProvisioner{Name: n.ProvisionerNameValue}
 }
 
-func (n *graphNodeMissingProvisioner) ProvisionerName() string {
+func (n *graphNodeProvisioner) ProvisionerName() string {
 	return n.ProvisionerNameValue
 }
 
 // GraphNodeFlattenable impl.
-func (n *graphNodeMissingProvisioner) Flatten(p []string) (dag.Vertex, error) {
+func (n *graphNodeProvisioner) Flatten(p []string) (dag.Vertex, error) {
-	return &graphNodeMissingProvisionerFlat{
+	return &graphNodeProvisionerFlat{
-		graphNodeMissingProvisioner: n,
+		graphNodeProvisioner: n,
-		PathValue:                   p,
+		PathValue:            p,
 	}, nil
 }
 
 // Same as graphNodeMissingProvisioner, but for flattening
-type graphNodeMissingProvisionerFlat struct {
+type graphNodeProvisionerFlat struct {
-	*graphNodeMissingProvisioner
+	*graphNodeProvisioner
 
 	PathValue []string
 }
 
-func (n *graphNodeMissingProvisionerFlat) Name() string {
+func (n *graphNodeProvisionerFlat) Name() string {
 	return fmt.Sprintf(
-		"%s.%s", modulePrefixStr(n.PathValue), n.graphNodeMissingProvisioner.Name())
+		"%s.%s", modulePrefixStr(n.PathValue), n.graphNodeProvisioner.Name())
 }
 
-func (n *graphNodeMissingProvisionerFlat) Path() []string {
+func (n *graphNodeProvisionerFlat) Path() []string {
 	return n.PathValue
 }
 
-func (n *graphNodeMissingProvisionerFlat) ProvisionerName() string {
+func (n *graphNodeProvisionerFlat) ProvisionerName() string {
 	return fmt.Sprintf(
 		"%s.%s", modulePrefixStr(n.PathValue),
-		n.graphNodeMissingProvisioner.ProvisionerName())
+		n.graphNodeProvisioner.ProvisionerName())
 }

terraform/transform_provisioner_test.go

@@ -19,14 +19,14 @@ func TestMissingProvisionerTransformer(t *testing.T) {
 	}
 
 	{
-		transform := &MissingProvisionerTransformer{Provisioners: []string{"foo"}}
+		transform := &MissingProvisionerTransformer{Provisioners: []string{"shell"}}
 		if err := transform.Transform(&g); err != nil {
 			t.Fatalf("err: %s", err)
 		}
 	}
 
 	{
-		transform := &CloseProvisionerTransformer{}
+		transform := &ProvisionerTransformer{}
 		if err := transform.Transform(&g); err != nil {
 			t.Fatalf("err: %s", err)
 		}
@@ -39,8 +39,8 @@ func TestMissingProvisionerTransformer(t *testing.T) {
 	}
 }
 
-func TestPruneProvisionerTransformer(t *testing.T) {
+func TestCloseProvisionerTransformer(t *testing.T) {
-	mod := testModule(t, "transform-provisioner-prune")
+	mod := testModule(t, "transform-provisioner-basic")
 
 	g := Graph{Path: RootModulePath}
 	{
@@ -51,8 +51,7 @@ func TestPruneProvisionerTransformer(t *testing.T) {
 	}
 
 	{
-		transform := &MissingProvisionerTransformer{
+		transform := &MissingProvisionerTransformer{Provisioners: []string{"shell"}}
-			Provisioners: []string{"foo", "bar"}}
 		if err := transform.Transform(&g); err != nil {
 			t.Fatalf("err: %s", err)
 		}
@@ -72,28 +71,20 @@ func TestPruneProvisionerTransformer(t *testing.T) {
 		}
 	}
 
-	{
-		transform := &PruneProvisionerTransformer{}
-		if err := transform.Transform(&g); err != nil {
-			t.Fatalf("err: %s", err)
-		}
-	}
-
 	actual := strings.TrimSpace(g.String())
-	expected := strings.TrimSpace(testTransformPruneProvisionerBasicStr)
+	expected := strings.TrimSpace(testTransformCloseProvisionerBasicStr)
 	if actual != expected {
 		t.Fatalf("bad:\n\n%s", actual)
 	}
 }
-
+func TestGraphNodeProvisioner_impl(t *testing.T) {
-func TestGraphNodeMissingProvisioner_impl(t *testing.T) {
+	var _ dag.Vertex = new(graphNodeProvisioner)
-	var _ dag.Vertex = new(graphNodeMissingProvisioner)
+	var _ dag.NamedVertex = new(graphNodeProvisioner)
-	var _ dag.NamedVertex = new(graphNodeMissingProvisioner)
+	var _ GraphNodeProvisioner = new(graphNodeProvisioner)
-	var _ GraphNodeProvisioner = new(graphNodeMissingProvisioner)
 }
 
-func TestGraphNodeMissingProvisioner_ProvisionerName(t *testing.T) {
+func TestGraphNodeProvisioner_ProvisionerName(t *testing.T) {
-	n := &graphNodeMissingProvisioner{ProvisionerNameValue: "foo"}
+	n := &graphNodeProvisioner{ProvisionerNameValue: "foo"}
 	if v := n.ProvisionerName(); v != "foo" {
 		t.Fatalf("bad: %#v", v)
 	}
@@ -101,15 +92,14 @@ func TestGraphNodeMissingProvisioner_ProvisionerName(t *testing.T) {
 
 const testTransformMissingProvisionerBasicStr = `
 aws_instance.web
-provisioner.foo
+  provisioner.shell
+provisioner.shell
+`
+
+const testTransformCloseProvisionerBasicStr = `
+aws_instance.web
+  provisioner.shell
+provisioner.shell
 provisioner.shell (close)
   aws_instance.web
 `
-
-const testTransformPruneProvisionerBasicStr = `
-aws_instance.web
-  provisioner.foo
-provisioner.foo
-provisioner.foo (close)
-  aws_instance.web
-`