From 1bec11472a5af89a8d2f82e4840e1ff91941c364 Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Thu, 4 Feb 2016 13:46:45 +0100 Subject: [PATCH] Cleaning up the PruneProvisionerTransformer And renamed some types so they better reflect what they are for. --- terraform/graph_builder.go | 6 +- terraform/graph_builder_test.go | 3 + terraform/transform_provisioner.go | 112 ++++++++++-------------- terraform/transform_provisioner_test.go | 49 +++++------ 4 files changed, 69 insertions(+), 101 deletions(-) diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go index 793dc904cf..e043e5cd11 100644 --- a/terraform/graph_builder.go +++ b/terraform/graph_builder.go @@ -146,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 - // only once because flattened modules might depend on empty - // providers. + // Prune the providers. This must happen only once because flattened + // modules might depend on empty providers. &PruneProviderTransformer{}, - &PruneProvisionerTransformer{}, // Create the destruction nodes &DestroyTransformer{FullDestroy: b.Destroy}, diff --git a/terraform/graph_builder_test.go b/terraform/graph_builder_test.go index dbc8ae61e1..13b3a906f0 100644 --- a/terraform/graph_builder_test.go +++ b/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 = ` diff --git a/terraform/transform_provisioner.go b/terraform/transform_provisioner.go index dc8513f955..2d86275dc8 100644 --- a/terraform/transform_provisioner.go +++ b/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() { - target := m[provisionerName] - if target == nil { + for _, p := range pv.ProvisionedBy() { + if m[p] == 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,41 +55,8 @@ func (t *ProvisionerTransformer) Transform(g *Graph) error { return err } -// 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 -// resources in the graph are evaluated. -type CloseProvisionerTransformer struct{} - -func (t *CloseProvisionerTransformer) Transform(g *Graph) error { - m := closeProvisionerVertexMap(g) - for _, v := range g.Vertices() { - if pv, ok := v.(GraphNodeProvisionerConsumer); ok { - for _, p := range pv.ProvisionedBy() { - source := m[p] - - if source == nil { - // Create a new graphNodeCloseProvisioner and add it to the graph - source = &graphNodeCloseProvisioner{ProvisionerNameValue: p} - g.Add(source) - - // Make sure we also add the new graphNodeCloseProvisioner to the map - // so we don't create and add any duplicate graphNodeCloseProvisioners. - m[p] = source - } - - g.Connect(dag.BasicEdge(source, v)) - } - } - } - - 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. +// for missing provisioners into the graph. type MissingProvisionerTransformer struct { // Provisioners is the list of provisioners we support. Provisioners []string @@ -126,29 +92,39 @@ func (t *MissingProvisionerTransformer) Transform(g *Graph) error { continue } - // Add our own missing provisioner node to the graph - m[p] = g.Add(&graphNodeMissingProvisioner{ProvisionerNameValue: p}) + // Add the missing provisioner node to the graph + m[p] = g.Add(&graphNodeProvisioner{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{} +// 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 +// resources in the graph are evaluated. +type CloseProvisionerTransformer struct{} -func (t *PruneProvisionerTransformer) Transform(g *Graph) error { +func (t *CloseProvisionerTransformer) Transform(g *Graph) error { + m := closeProvisionerVertexMap(g) for _, v := range g.Vertices() { - // We only care about the provisioners - if _, ok := v.(GraphNodeProvisioner); !ok { - continue - } + if pv, ok := v.(GraphNodeProvisionerConsumer); ok { + for _, p := range pv.ProvisionedBy() { + source := m[p] - // Does anything depend on this? If not, then prune it. - if s := g.UpEdges(v); s.Len() == 0 { - g.Remove(v) + if source == nil { + // Create a new graphNodeCloseProvisioner and add it to the graph + source = &graphNodeCloseProvisioner{ProvisionerNameValue: p} + g.Add(source) + + // Make sure we also add the new graphNodeCloseProvisioner to the map + // so we don't create and add any duplicate graphNodeCloseProvisioners. + m[p] = source + } + + g.Connect(dag.BasicEdge(source, v)) + } } } @@ -194,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) { - return &graphNodeMissingProvisionerFlat{ - graphNodeMissingProvisioner: n, - PathValue: p, +func (n *graphNodeProvisioner) Flatten(p []string) (dag.Vertex, error) { + return &graphNodeProvisionerFlat{ + graphNodeProvisioner: n, + PathValue: p, }, nil } // Same as graphNodeMissingProvisioner, but for flattening -type graphNodeMissingProvisionerFlat struct { - *graphNodeMissingProvisioner +type graphNodeProvisionerFlat struct { + *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()) } diff --git a/terraform/transform_provisioner_test.go b/terraform/transform_provisioner_test.go index 823835372d..38874bec82 100644 --- a/terraform/transform_provisioner_test.go +++ b/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) { - mod := testModule(t, "transform-provisioner-prune") +func TestCloseProvisionerTransformer(t *testing.T) { + mod := testModule(t, "transform-provisioner-basic") g := Graph{Path: RootModulePath} { @@ -51,8 +51,7 @@ func TestPruneProvisionerTransformer(t *testing.T) { } { - transform := &MissingProvisionerTransformer{ - Provisioners: []string{"foo", "bar"}} + transform := &MissingProvisionerTransformer{Provisioners: []string{"shell"}} 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 TestGraphNodeMissingProvisioner_impl(t *testing.T) { - var _ dag.Vertex = new(graphNodeMissingProvisioner) - var _ dag.NamedVertex = new(graphNodeMissingProvisioner) - var _ GraphNodeProvisioner = new(graphNodeMissingProvisioner) +func TestGraphNodeProvisioner_impl(t *testing.T) { + var _ dag.Vertex = new(graphNodeProvisioner) + var _ dag.NamedVertex = new(graphNodeProvisioner) + var _ GraphNodeProvisioner = new(graphNodeProvisioner) } -func TestGraphNodeMissingProvisioner_ProvisionerName(t *testing.T) { - n := &graphNodeMissingProvisioner{ProvisionerNameValue: "foo"} +func TestGraphNodeProvisioner_ProvisionerName(t *testing.T) { + n := &graphNodeProvisioner{ProvisionerNameValue: "foo"} if v := n.ProvisionerName(); v != "foo" { t.Fatalf("bad: %#v", v) } @@ -101,14 +92,14 @@ func TestGraphNodeMissingProvisioner_ProvisionerName(t *testing.T) { const testTransformMissingProvisionerBasicStr = ` aws_instance.web + 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 -`