From b62640d2d5bb36242a5d5238094e35907ac5069c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Jul 2020 11:08:14 -0400 Subject: [PATCH 1/2] update output destroy test to reference expander Have the output reference the expansion of a resource (via the whole resource object), so that we can be sure we don't attempt to evaluate that expansion during destroy. --- terraform/context_apply_test.go | 19 ++++++++++--- .../testdata/apply-destroy-outputs/main.tf | 28 ++++++++++++++----- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 7b2e23b18..9c3524ebe 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -6275,13 +6275,24 @@ func TestContext2Apply_destroyWithModuleVariableAndCountNested(t *testing.T) { func TestContext2Apply_destroyOutputs(t *testing.T) { m := testModule(t, "apply-destroy-outputs") - p := testProvider("aws") + p := testProvider("test") p.ApplyFn = testApplyFn p.DiffFn = testDiffFn + + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + // add the required id + m := req.Config.AsValueMap() + m["id"] = cty.StringVal("foo") + + return providers.ReadDataSourceResponse{ + State: cty.ObjectVal(m), + } + } + ctx := testContext2(t, &ContextOpts{ Config: m, Providers: map[addrs.Provider]providers.Factory{ - addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), }, }) @@ -6302,7 +6313,7 @@ func TestContext2Apply_destroyOutputs(t *testing.T) { State: state, Config: m, Providers: map[addrs.Provider]providers.Factory{ - addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), }, }) @@ -6326,7 +6337,7 @@ func TestContext2Apply_destroyOutputs(t *testing.T) { State: state, Config: m, Providers: map[addrs.Provider]providers.Factory{ - addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), }, }) if _, diags := ctx.Plan(); diags.HasErrors() { diff --git a/terraform/testdata/apply-destroy-outputs/main.tf b/terraform/testdata/apply-destroy-outputs/main.tf index 97ec848e4..b50c14fd3 100644 --- a/terraform/testdata/apply-destroy-outputs/main.tf +++ b/terraform/testdata/apply-destroy-outputs/main.tf @@ -1,12 +1,26 @@ -resource "aws_instance" "foo" { - num = "2" +data "test_data_source" "foo" { + foo = "ok" } -resource "aws_instance" "bar" { - foo = "{aws_instance.foo.num}" - dep = "foo" +locals { + l = [ + { + name = data.test_data_source.foo.id + val = "null" + }, + ] + + m = { for v in local.l : + v.name => v + } } -output "foo" { - value = "${aws_instance.foo.id}" +resource "test_instance" "bar" { + for_each = local.m + foo = format("%s", each.value.name) + dep = each.value.val +} + +output "out" { + value = test_instance.bar } From 2555f6f9882ab3f6edefd2d5b827192fb253359b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Jul 2020 11:10:15 -0400 Subject: [PATCH 2/2] remove root output eval nodes from destroy If we're adding a node to remove a root output from the state, the output itself does not need to be re-evaluated. The exception for root outputs caused them to be missed when we refactored resource destruction to only use the existing state. --- terraform/graph_builder_apply.go | 2 +- terraform/transform_destroy_edge.go | 8 ++++---- terraform/transform_output.go | 7 ++++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index ffb4b477d..6cb2f56ec 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -165,7 +165,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // directly. A destroy is identical to a normal apply, except for the // fact that we also have configuration to evaluate. While the rest of // the unused nodes can be programmatically pruned (via - // pruneUnusedNodesTransformer), root module outputs only have an + // pruneUnusedNodesTransformer), root module outputs always have an // implied dependency on remote state. This means that if they exist in // the configuration, the only signal to remove them is via the destroy // command itself. diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 5516d8413..1d28c1118 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -207,10 +207,10 @@ func (t *pruneUnusedNodesTransformer) Transform(g *Graph) error { modules = append(modules, mod) } - // Sort them by path length, longest first, so that start with the deepest - // modules. The order of modules at the same tree level doesn't matter, we - // just need to ensure that child modules are processed before parent - // modules. + // Sort them by path length, longest first, so that we start with the + // deepest modules. The order of modules at the same tree level doesn't + // matter, we just need to ensure that child modules are processed before + // parent modules. sort.Slice(modules, func(i, j int) bool { return len(modules[i].addr) > len(modules[j].addr) }) diff --git a/terraform/transform_output.go b/terraform/transform_output.go index b926b2fd1..d1b130c63 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -88,13 +88,14 @@ func (t *destroyRootOutputTransformer) Transform(g *Graph) error { deps := g.UpEdges(v) - // the destroy node must depend on the eval node - deps.Add(v) - for _, d := range deps { log.Printf("[TRACE] %s depends on %s", node.Name(), dag.VertexName(d)) g.Connect(dag.BasicEdge(node, d)) } + + // We no longer need the expand node, since we intend to remove this + // output from the state. + g.Remove(v) } return nil }