From 2b4695eecb1c0e28c41d950b669d9de3aba2880a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 21 Aug 2019 19:06:25 -0400 Subject: [PATCH 1/2] only create one provisioner instance per type There's no reason to start individual provisioners per module path, as they are not configured per module (or independently at all for that matter). --- terraform/eval_context_builtin.go | 13 +++------- terraform/path.go | 17 ------------- terraform/transform_provisioner.go | 40 ++++++------------------------ 3 files changed, 11 insertions(+), 59 deletions(-) delete mode 100644 terraform/path.go diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 20b37933d047..3ca02ad8b771 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -225,14 +225,12 @@ func (ctx *BuiltinEvalContext) InitProvisioner(n string) (provisioners.Interface ctx.ProvisionerLock.Lock() defer ctx.ProvisionerLock.Unlock() - key := PathObjectCacheKey(ctx.Path(), n) - - p, err := ctx.Components.ResourceProvisioner(n, key) + p, err := ctx.Components.ResourceProvisioner(n, "") if err != nil { return nil, err } - ctx.ProvisionerCache[key] = p + ctx.ProvisionerCache[n] = p return p, nil } @@ -243,8 +241,7 @@ func (ctx *BuiltinEvalContext) Provisioner(n string) provisioners.Interface { ctx.ProvisionerLock.Lock() defer ctx.ProvisionerLock.Unlock() - key := PathObjectCacheKey(ctx.Path(), n) - return ctx.ProvisionerCache[key] + return ctx.ProvisionerCache[n] } func (ctx *BuiltinEvalContext) ProvisionerSchema(n string) *configschema.Block { @@ -259,9 +256,7 @@ func (ctx *BuiltinEvalContext) CloseProvisioner(n string) error { ctx.ProvisionerLock.Lock() defer ctx.ProvisionerLock.Unlock() - key := PathObjectCacheKey(ctx.Path(), n) - - prov := ctx.ProvisionerCache[key] + prov := ctx.ProvisionerCache[n] if prov != nil { return prov.Close() } diff --git a/terraform/path.go b/terraform/path.go deleted file mode 100644 index 9757446bb6ce..000000000000 --- a/terraform/path.go +++ /dev/null @@ -1,17 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/terraform/addrs" -) - -// PathObjectCacheKey is like PathCacheKey but includes an additional name -// to be included in the key, for module-namespaced objects. -// -// The result of this function is guaranteed unique for any distinct pair -// of path and name, but is not guaranteed to be in any particular format -// and in particular should never be shown to end-users. -func PathObjectCacheKey(path addrs.ModuleInstance, objectName string) string { - return fmt.Sprintf("%s|%s", path.String(), objectName) -} diff --git a/terraform/transform_provisioner.go b/terraform/transform_provisioner.go index fe4cf0e9e850..b310266556d1 100644 --- a/terraform/transform_provisioner.go +++ b/terraform/transform_provisioner.go @@ -4,8 +4,6 @@ import ( "fmt" "log" - "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/dag" ) @@ -43,16 +41,15 @@ func (t *ProvisionerTransformer) Transform(g *Graph) error { for _, v := range g.Vertices() { if pv, ok := v.(GraphNodeProvisionerConsumer); ok { for _, p := range pv.ProvisionedBy() { - key := provisionerMapKey(p, pv) - if m[key] == nil { + if m[p] == nil { err = multierror.Append(err, fmt.Errorf( "%s: provisioner %s couldn't be found", dag.VertexName(v), p)) continue } - log.Printf("[TRACE] ProvisionerTransformer: %s is provisioned by %s (%q)", dag.VertexName(v), key, dag.VertexName(m[key])) - g.Connect(dag.BasicEdge(v, m[key])) + log.Printf("[TRACE] ProvisionerTransformer: %s is provisioned by %s (%q)", dag.VertexName(v), p, dag.VertexName(m[p])) + g.Connect(dag.BasicEdge(v, m[p])) } } } @@ -85,18 +82,8 @@ func (t *MissingProvisionerTransformer) Transform(g *Graph) error { continue } - // If this node has a subpath, then we use that as a prefix - // into our map to check for an existing provider. - path := addrs.RootModuleInstance - if sp, ok := pv.(GraphNodeSubPath); ok { - path = sp.Path() - } - for _, p := range pv.ProvisionedBy() { - // Build the key for storing in the map - key := provisionerMapKey(p, pv) - - if _, ok := m[key]; ok { + if _, ok := m[p]; ok { // This provisioner already exists as a configure node continue } @@ -110,12 +97,11 @@ func (t *MissingProvisionerTransformer) Transform(g *Graph) error { // Build the vertex var newV dag.Vertex = &NodeProvisioner{ NameValue: p, - PathValue: path, } // Add the missing provisioner node to the graph - m[key] = g.Add(newV) - log.Printf("[TRACE] MissingProviderTransformer: added implicit provisioner %s, first implied by %s", key, dag.VertexName(v)) + m[p] = g.Add(newV) + log.Printf("[TRACE] MissingProviderTransformer: added implicit provisioner %s, first implied by %s", p, dag.VertexName(v)) } } @@ -153,23 +139,11 @@ func (t *CloseProvisionerTransformer) Transform(g *Graph) error { return nil } -// provisionerMapKey is a helper that gives us the key to use for the -// maps returned by things such as provisionerVertexMap. -func provisionerMapKey(k string, v dag.Vertex) string { - pathPrefix := "" - if sp, ok := v.(GraphNodeSubPath); ok { - pathPrefix = sp.Path().String() + "." - } - - return pathPrefix + k -} - func provisionerVertexMap(g *Graph) map[string]dag.Vertex { m := make(map[string]dag.Vertex) for _, v := range g.Vertices() { if pv, ok := v.(GraphNodeProvisioner); ok { - key := provisionerMapKey(pv.ProvisionerName(), v) - m[key] = v + m[pv.ProvisionerName()] = v } } From b1025a9d29c653d490c72f6e046fbd87fcd358a2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 21 Aug 2019 19:32:14 -0400 Subject: [PATCH 2/2] update tests to reflect correct provisioners We no longer create new provisioners for every module. --- terraform/graph_builder_apply_test.go | 4 ++-- terraform/transform_provisioner_test.go | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 6050be7df580..ac65751e6048 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -478,12 +478,11 @@ const testApplyGraphBuilderStr = ` meta.count-boundary (EachMode fixup) module.child.test_object.other test_object.other -module.child.provisioner.test module.child.test_object.create module.child.test_object.create (prepare state) module.child.test_object.create (prepare state) - module.child.provisioner.test provider.test + provisioner.test module.child.test_object.other module.child.test_object.create module.child.test_object.other (prepare state) @@ -493,6 +492,7 @@ provider.test provider.test (close) module.child.test_object.other test_object.other +provisioner.test provisioner.test (close) module.child.test_object.create root diff --git a/terraform/transform_provisioner_test.go b/terraform/transform_provisioner_test.go index dd2714f33532..eecd677888a8 100644 --- a/terraform/transform_provisioner_test.go +++ b/terraform/transform_provisioner_test.go @@ -188,8 +188,7 @@ const testTransformMissingProvisionerModuleStr = ` aws_instance.foo provisioner.shell module.child.aws_instance.foo - module.child.provisioner.shell -module.child.provisioner.shell + provisioner.shell provisioner.shell `