From 45f2a61bdb22c71202baf6eaa6d620fcdbdd1c24 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 13:13:50 -0500 Subject: [PATCH 01/17] do not connect references from destroy nodes Destroy nodes only require their own state to evaluate. Do not connect any of their references in the graph. --- terraform/transform_reference.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index df34a3cea8cc..945479bcf353 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -80,6 +80,12 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { // Find the things that reference things and connect them for _, v := range vs { + if _, ok := v.(GraphNodeDestroyer); ok { + // destroy nodes references are not connected, since they can only + // use their own state. + continue + } + parents, _ := m.References(v) parentsDbg := make([]string, len(parents)) for i, v := range parents { From 8b5522a090bdee8ca5000d29defc45bab59d8359 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 13:15:13 -0500 Subject: [PATCH 02/17] do not attempt to find more destroy dependencies The requires destroy dependencies are what is stored in the state, and have already been connected. There is no need to search further, since new nodes from the config may interfere with the original destroy ordering. --- terraform/transform_destroy_edge.go | 153 ---------------------------- 1 file changed, 153 deletions(-) diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index f524292293c3..690effd2e56b 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -165,159 +165,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { } } - // This is strange but is the easiest way to get the dependencies - // of a node that is being destroyed. We use another graph to make sure - // the resource is in the graph and ask for references. We have to do this - // because the node that is being destroyed may NOT be in the graph. - // - // Example: resource A is force new, then destroy A AND create A are - // in the graph. BUT if resource A is just pure destroy, then only - // destroy A is in the graph, and create A is not. - providerFn := func(a *NodeAbstractProvider) dag.Vertex { - return &NodeApplyableProvider{NodeAbstractProvider: a} - } - steps := []GraphTransformer{ - // Add the local values - &LocalTransformer{Config: t.Config}, - - // Add outputs and metadata - &OutputTransformer{Config: t.Config}, - &AttachResourceConfigTransformer{Config: t.Config}, - &AttachStateTransformer{State: t.State}, - - // Add all the variables. We can depend on resources through - // variables due to module parameters, and we need to properly - // determine that. - &RootVariableTransformer{Config: t.Config}, - &ModuleVariableTransformer{Config: t.Config}, - - TransformProviders(nil, providerFn, t.Config), - - // Must attach schemas before ReferenceTransformer so that we can - // analyze the configuration to find references. - &AttachSchemaTransformer{Schemas: t.Schemas}, - - &ReferenceTransformer{}, - } - - // Go through all the nodes being destroyed and create a graph. - // The resulting graph is only of things being CREATED. For example, - // following our example, the resulting graph would be: - // - // A, B (with no edges) - // - var tempG Graph - var tempDestroyed []dag.Vertex - for d := range destroyers { - // d is the string key for the resource being destroyed. We actually - // want the address value, which we stashed earlier. - addr := destroyerAddrs[d] - - // This part is a little bit weird but is the best way to - // find the dependencies we need to: build a graph and use the - // attach config and state transformers then ask for references. - abstract := NewNodeAbstractResourceInstance(addr) - tempG.Add(abstract) - tempDestroyed = append(tempDestroyed, abstract) - - // We also add the destroy version here since the destroy can - // depend on things that the creation doesn't (destroy provisioners). - destroy := &NodeDestroyResourceInstance{NodeAbstractResourceInstance: abstract} - tempG.Add(destroy) - tempDestroyed = append(tempDestroyed, destroy) - } - - // Run the graph transforms so we have the information we need to - // build references. - log.Printf("[TRACE] DestroyEdgeTransformer: constructing temporary graph for analysis of references, starting from:\n%s", tempG.StringWithNodeTypes()) - for _, s := range steps { - log.Printf("[TRACE] DestroyEdgeTransformer: running %T on temporary graph", s) - if err := s.Transform(&tempG); err != nil { - log.Printf("[TRACE] DestroyEdgeTransformer: %T failed: %s", s, err) - return err - } - } - log.Printf("[TRACE] DestroyEdgeTransformer: temporary reference graph:\n%s", tempG.String()) - - // Go through all the nodes in the graph and determine what they - // depend on. - for _, v := range tempDestroyed { - // Find all ancestors of this to determine the edges we'll depend on - vs, err := tempG.Ancestors(v) - if err != nil { - return err - } - - refs := make([]dag.Vertex, 0, vs.Len()) - for _, raw := range vs.List() { - refs = append(refs, raw.(dag.Vertex)) - } - - refNames := make([]string, len(refs)) - for i, ref := range refs { - refNames[i] = dag.VertexName(ref) - } - log.Printf( - "[TRACE] DestroyEdgeTransformer: creation node %q references %s", - dag.VertexName(v), refNames) - - // If we have no references, then we won't need to do anything - if len(refs) == 0 { - continue - } - - // Get the destroy node for this. In the example of our struct, - // we are currently at B and we're looking for B_d. - rn, ok := v.(GraphNodeResourceInstance) - if !ok { - log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s, since it's not a resource", dag.VertexName(v)) - continue - } - - addr := rn.ResourceInstanceAddr() - dns := destroyers[addr.String()] - - // We have dependencies, check if any are being destroyed - // to build the list of things that we must depend on! - // - // In the example of the struct, if we have: - // - // B_d => A_d => A => B - // - // Then at this point in the algorithm we started with B_d, - // we built B (to get dependencies), and we found A. We're now looking - // to see if A_d exists. - var depDestroyers []dag.Vertex - for _, v := range refs { - rn, ok := v.(GraphNodeResourceInstance) - if !ok { - continue - } - - addr := rn.ResourceInstanceAddr() - key := addr.String() - if ds, ok := destroyers[key]; ok { - for _, d := range ds { - depDestroyers = append(depDestroyers, d.(dag.Vertex)) - log.Printf( - "[TRACE] DestroyEdgeTransformer: destruction of %q depends on %s", - key, dag.VertexName(d)) - } - } - } - - // Go through and make the connections. Use the variable - // names "a_d" and "b_d" to reference our example. - for _, a_d := range dns { - for _, b_d := range depDestroyers { - if b_d != a_d { - log.Printf("[TRACE] DestroyEdgeTransformer: %q depends on %q", dag.VertexName(b_d), dag.VertexName(a_d)) - g.Connect(dag.BasicEdge(b_d, a_d)) - } - } - } - } - return t.pruneResources(g) } From 451190a5e646de1708cb524c7a604e92a7220a80 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 13:20:44 -0500 Subject: [PATCH 03/17] remove DestroyEdge This special edge type is no longer used. While we still have the option of encoding more meaning into the edged themselves, having one special edge type used only in one specific case was easily overlooked, as dag.BasicEdge is assumed in all other cases. --- terraform/edge_destroy.go | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100644 terraform/edge_destroy.go diff --git a/terraform/edge_destroy.go b/terraform/edge_destroy.go deleted file mode 100644 index bc9d638aadf2..000000000000 --- a/terraform/edge_destroy.go +++ /dev/null @@ -1,17 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/terraform/dag" -) - -// DestroyEdge is an edge that represents a standard "destroy" relationship: -// Target depends on Source because Source is destroying. -type DestroyEdge struct { - S, T dag.Vertex -} - -func (e *DestroyEdge) Hashcode() interface{} { return fmt.Sprintf("%p-%p", e.S, e.T) } -func (e *DestroyEdge) Source() dag.Vertex { return e.S } -func (e *DestroyEdge) Target() dag.Vertex { return e.T } From 84d1f5c688f07fc40580015c67e2ef20b1e87e78 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 12 Dec 2019 16:26:42 -0500 Subject: [PATCH 04/17] convert destroy provisioner warnings to errors --- configs/parser_config_test.go | 59 +++++++++++++++++++ configs/provisioner.go | 4 +- .../destroy-provisioners.tf | 6 +- 3 files changed, 64 insertions(+), 5 deletions(-) rename configs/testdata/{warning-files => error-files}/destroy-provisioners.tf (69%) diff --git a/configs/parser_config_test.go b/configs/parser_config_test.go index 13d90fed69b3..2fe645ed7676 100644 --- a/configs/parser_config_test.go +++ b/configs/parser_config_test.go @@ -224,3 +224,62 @@ func TestParserLoadConfigFileWarning(t *testing.T) { }) } } + +// TestParseLoadConfigFileError is a test that verifies files from +// testdata/warning-files produce particular warnings. +// +// This test does not verify that reading these files produces the correct +// file element contents in spite of those warnings. More detailed assertions +// may be made on some subset of these configuration files in other tests. +func TestParserLoadConfigFileError(t *testing.T) { + files, err := ioutil.ReadDir("testdata/error-files") + if err != nil { + t.Fatal(err) + } + + for _, info := range files { + name := info.Name() + t.Run(name, func(t *testing.T) { + src, err := ioutil.ReadFile(filepath.Join("testdata/error-files", name)) + if err != nil { + t.Fatal(err) + } + + // First we'll scan the file to see what warnings are expected. + // That's declared inside the files themselves by using the + // string "WARNING: " somewhere on each line that is expected + // to produce a warning, followed by the expected warning summary + // text. A single-line comment (with #) is the main way to do that. + const marker = "ERROR: " + sc := bufio.NewScanner(bytes.NewReader(src)) + wantErrors := make(map[int]string) + lineNum := 1 + for sc.Scan() { + lineText := sc.Text() + if idx := strings.Index(lineText, marker); idx != -1 { + summaryText := lineText[idx+len(marker):] + wantErrors[lineNum] = summaryText + } + lineNum++ + } + + parser := testParser(map[string]string{ + name: string(src), + }) + + _, diags := parser.LoadConfigFile(name) + + gotErrors := make(map[int]string) + for _, diag := range diags { + if diag.Severity != hcl.DiagError || diag.Subject == nil { + continue + } + gotErrors[diag.Subject.Start.Line] = diag.Summary + } + + if diff := cmp.Diff(wantErrors, gotErrors); diff != "" { + t.Errorf("wrong errors\n%s", diff) + } + }) + } +} diff --git a/configs/provisioner.go b/configs/provisioner.go index 6f380860a848..769382513947 100644 --- a/configs/provisioner.go +++ b/configs/provisioner.go @@ -147,8 +147,8 @@ func onlySelfRefs(body hcl.Body) hcl.Diagnostics { if !valid { diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "External references from destroy provisioners are deprecated", + Severity: hcl.DiagError, + Summary: "Invalid reference from destroy provisioner", Detail: "Destroy-time provisioners and their connection configurations may only " + "reference attributes of the related resource, via 'self', 'count.index', " + "or 'each.key'.\n\nReferences to other resources during the destroy phase " + diff --git a/configs/testdata/warning-files/destroy-provisioners.tf b/configs/testdata/error-files/destroy-provisioners.tf similarity index 69% rename from configs/testdata/warning-files/destroy-provisioners.tf rename to configs/testdata/error-files/destroy-provisioners.tf index c6c28b823dae..4831b5302f60 100644 --- a/configs/testdata/warning-files/destroy-provisioners.tf +++ b/configs/testdata/error-files/destroy-provisioners.tf @@ -5,7 +5,7 @@ locals { resource "null_resource" "a" { connection { host = self.hostname - user = local.user # WARNING: External references from destroy provisioners are deprecated + user = local.user # ERROR: Invalid reference from destroy provisioner } provisioner "remote-exec" { @@ -36,9 +36,9 @@ resource "null_resource" "b" { when = destroy connection { host = self.hostname - user = local.user # WARNING: External references from destroy provisioners are deprecated + user = local.user # ERROR: Invalid reference from destroy provisioner } - command = "echo ${local.name}" # WARNING: External references from destroy provisioners are deprecated + command = "echo ${local.name}" # ERROR: Invalid reference from destroy provisioner } } From b5517b53ec49dd17223b0b2e76db706253f4139f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Nov 2019 17:31:38 -0500 Subject: [PATCH 05/17] simplify CBD transformation Start by removing the DestroyEdge type altogether. This is only used to detect the natural edge between a resource's create and destroy nodes, but that's not necessary for any transformations. The custom edge type also interferes with normal graph manipulations, because you can't delete an arbitrary edge without knowing the type, so deletion of the edge based only on the endpoints is often done incorrectly. The dag package itself does this incorrectly in TransitiveReduction, which always assumes the BasicEdge type. Now that inter-resource destroy dependencies are already connected in the DestroyEdgeTransformer (from the stored deps in state), there's no need to search out all dependant resources in the CBD transformation, as they should all be connected. This makes the CBD transformation rule quite simple: reverse any edges from create nodes. --- terraform/transform_destroy_cbd.go | 163 ++-------------------------- terraform/transform_destroy_edge.go | 2 +- terraform/transform_reference.go | 2 +- 3 files changed, 12 insertions(+), 155 deletions(-) diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index 410a709ea40e..40474a0b4d50 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -145,14 +145,12 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { } // Go through and reverse any destroy edges - destroyMap := make(map[string][]dag.Vertex) for _, v := range g.Vertices() { dn, ok := v.(GraphNodeDestroyerCBD) if !ok { continue } - dern, ok := v.(GraphNodeDestroyer) - if !ok { + if _, ok = v.(GraphNodeDestroyer); !ok { continue } @@ -162,158 +160,17 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { // Find the resource edges for _, e := range g.EdgesTo(v) { - switch de := e.(type) { - case *DestroyEdge: - // we need to invert the destroy edge from the create node - log.Printf("[TRACE] CBDEdgeTransformer: inverting edge: %s => %s", - dag.VertexName(de.Source()), dag.VertexName(de.Target())) - - // Found it! Invert. - g.RemoveEdge(de) - applyNode := de.Source() - destroyNode := de.Target() - g.Connect(&DestroyEdge{S: destroyNode, T: applyNode}) - default: - // We cannot have any direct dependencies from creators when - // the node is CBD without inducing a cycle. - if _, ok := e.Source().(GraphNodeCreator); ok { - log.Printf("[TRACE] CBDEdgeTransformer: removing non DestroyEdge to CBD destroy node: %s => %s", dag.VertexName(e.Source()), dag.VertexName(e.Target())) - g.RemoveEdge(e) - } + src := e.Source() + + // If source is a create node, invert the edge. + // This covers both the node's own creator, as well as reversing + // any dependants' edges. + if _, ok := src.(GraphNodeCreator); ok { + log.Printf("[TRACE] CBDEdgeTransformer: reversing edge %s -> %s", dag.VertexName(src), dag.VertexName(v)) + g.RemoveEdge(e) + g.Connect(dag.BasicEdge(v, src)) } } - - // If the address has an index, we strip that. Our depMap creation - // graph doesn't expand counts so we don't currently get _exact_ - // dependencies. One day when we limit dependencies more exactly - // this will have to change. We have a test case covering this - // (depNonCBDCountBoth) so it'll be caught. - addr := dern.DestroyAddr() - key := addr.ContainingResource().String() - - // Add this to the list of nodes that we need to fix up - // the edges for (step 2 above in the docs). - destroyMap[key] = append(destroyMap[key], v) - } - - // If we have no CBD nodes, then our work here is done - if len(destroyMap) == 0 { - return nil } - - // We have CBD nodes. We now have to move on to the much more difficult - // task of connecting dependencies of the creation side of the destroy - // to the destruction node. The easiest way to explain this is an example: - // - // Given a pre-destroy dependence of: A => B - // And A has CBD set. - // - // The resulting graph should be: A => B => A_d - // - // They key here is that B happens before A is destroyed. This is to - // facilitate the primary purpose for CBD: making sure that downstreams - // are properly updated to avoid downtime before the resource is destroyed. - depMap, err := t.depMap(g, destroyMap) - if err != nil { - return err - } - - // We now have the mapping of resource addresses to the destroy - // nodes they need to depend on. We now go through our own vertices to - // find any matching these addresses and make the connection. - for _, v := range g.Vertices() { - // We're looking for creators - rn, ok := v.(GraphNodeCreator) - if !ok { - continue - } - - // Get the address - addr := rn.CreateAddr() - - // If the address has an index, we strip that. Our depMap creation - // graph doesn't expand counts so we don't currently get _exact_ - // dependencies. One day when we limit dependencies more exactly - // this will have to change. We have a test case covering this - // (depNonCBDCount) so it'll be caught. - key := addr.ContainingResource().String() - - // If there is nothing this resource should depend on, ignore it - dns, ok := depMap[key] - if !ok { - continue - } - - // We have nodes! Make the connection - for _, dn := range dns { - log.Printf("[TRACE] CBDEdgeTransformer: destroy depends on dependence: %s => %s", - dag.VertexName(dn), dag.VertexName(v)) - g.Connect(dag.BasicEdge(dn, v)) - } - } - return nil } - -func (t *CBDEdgeTransformer) depMap(g *Graph, destroyMap map[string][]dag.Vertex) (map[string][]dag.Vertex, error) { - // Build the list of destroy nodes that each resource address should depend - // on. For example, when we find B, we map the address of B to A_d in the - // "depMap" variable below. - - // Use a nested map to remove duplicate edges. - depMap := make(map[string]map[dag.Vertex]struct{}) - for _, v := range g.Vertices() { - // We're looking for resources. - rn, ok := v.(GraphNodeResource) - if !ok { - continue - } - - // Get the address - addr := rn.ResourceAddr() - key := addr.String() - - // Get the destroy nodes that are destroying this resource. - // If there aren't any, then we don't need to worry about - // any connections. - dns, ok := destroyMap[key] - if !ok { - continue - } - - // Get the nodes that depend on this on. In the example above: - // finding B in A => B. Since dependencies can span modules, walk all - // descendents of the resource. - des, _ := g.Descendents(v) - for _, v := range des.List() { - // We're looking for resources. - rn, ok := v.(GraphNodeResource) - if !ok { - continue - } - - // Keep track of the destroy nodes that this address - // needs to depend on. - key := rn.ResourceAddr().String() - - deps, ok := depMap[key] - if !ok { - deps = make(map[dag.Vertex]struct{}) - } - - for _, d := range dns { - deps[d] = struct{}{} - } - depMap[key] = deps - } - } - - result := map[string][]dag.Vertex{} - for k, m := range depMap { - for v := range m { - result[k] = append(result[k], v) - } - } - - return result, nil -} diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 690effd2e56b..d954d2c048db 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -151,7 +151,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { "[TRACE] DestroyEdgeTransformer: connecting creator %q with destroyer %q", dag.VertexName(a), dag.VertexName(a_d)) - g.Connect(&DestroyEdge{S: a, T: a_d}) + g.Connect(dag.BasicEdge(a, a_d)) // Attach the destroy node to the creator // There really shouldn't be more than one destroyer, but even if diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 945479bcf353..ceff6914875b 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -205,7 +205,7 @@ func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { log.Printf("[TRACE] output dep: %s", dag.VertexName(target)) g.RemoveEdge(e) - g.Connect(&DestroyEdge{S: target, T: v}) + g.Connect(dag.BasicEdge(target, v)) } } From 681d19762865d823f27ca6f7935e581a213a1f52 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 15:00:48 -0500 Subject: [PATCH 06/17] fix DestroyEdgeTransformer tests The tests require working node implementations with real state. --- terraform/transform_destroy_edge_test.go | 247 +++++++++++++---------- 1 file changed, 141 insertions(+), 106 deletions(-) diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index 01059db0291f..9659efeabc6a 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/terraform/transform_destroy_edge_test.go @@ -5,32 +5,37 @@ import ( "testing" "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/states" ) func TestDestroyEdgeTransformer_basic(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.B"}) - tf := &DestroyEdgeTransformer{ - Config: testModule(t, "transform-destroy-edge-basic"), - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformDestroyEdgeBasicStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) + g.Add(testDestroyNode("test_object.A")) + g.Add(testDestroyNode("test_object.B")) + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) } -} -func TestDestroyEdgeTransformer_create(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.B"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.A"}) tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-basic"), Schemas: simpleTestSchemas(), @@ -40,7 +45,7 @@ func TestDestroyEdgeTransformer_create(t *testing.T) { } actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformDestroyEdgeCreatorStr) + expected := strings.TrimSpace(testTransformDestroyEdgeBasicStr) if actual != expected { t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) } @@ -48,9 +53,46 @@ func TestDestroyEdgeTransformer_create(t *testing.T) { func TestDestroyEdgeTransformer_multi(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.B"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.C"}) + g.Add(testDestroyNode("test_object.A")) + g.Add(testDestroyNode("test_object.B")) + g.Add(testDestroyNode("test_object.C")) + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.C").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"C","test_string":"x"}`), + Dependencies: []addrs.AbsResource{ + mustResourceAddr("test_object.A"), + mustResourceAddr("test_object.B"), + }, + }, + mustProviderConfig("provider.test"), + ) + + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) + } + tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-multi"), Schemas: simpleTestSchemas(), @@ -68,7 +110,7 @@ func TestDestroyEdgeTransformer_multi(t *testing.T) { func TestDestroyEdgeTransformer_selfRef(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) + g.Add(testDestroyNode("test_object.A")) tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-self-ref"), Schemas: simpleTestSchemas(), @@ -86,8 +128,33 @@ func TestDestroyEdgeTransformer_selfRef(t *testing.T) { func TestDestroyEdgeTransformer_module(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "module.child.test_object.b"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.a"}) + g.Add(testDestroyNode("module.child.test_object.b")) + g.Add(testDestroyNode("test_object.a")) + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.a").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.b")}, + }, + mustProviderConfig("provider.test"), + ) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.b").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b","test_string":"x"}`), + }, + mustProviderConfig("provider.test"), + ) + + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) + } + tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-module"), Schemas: simpleTestSchemas(), @@ -105,9 +172,46 @@ func TestDestroyEdgeTransformer_module(t *testing.T) { func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "module.child.test_object.a"}) - g.Add(&graphNodeDestroyerTest{AddrString: "module.child.test_object.b"}) - g.Add(&graphNodeDestroyerTest{AddrString: "module.child.test_object.c"}) + g.Add(testDestroyNode("module.child.test_object.a")) + g.Add(testDestroyNode("module.child.test_object.b")) + g.Add(testDestroyNode("module.child.test_object.c")) + + state := states.NewState() + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.a").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a"}`), + }, + mustProviderConfig("provider.test"), + ) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.b").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b","test_string":"x"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.a")}, + }, + mustProviderConfig("provider.test"), + ) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.c").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"c","test_string":"x"}`), + Dependencies: []addrs.AbsResource{ + mustResourceAddr("module.child.test_object.a"), + mustResourceAddr("module.child.test_object.b"), + }, + }, + mustProviderConfig("provider.test"), + ) + + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) + } + tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-module-only"), Schemas: simpleTestSchemas(), @@ -130,86 +234,17 @@ module.child.test_object.c (destroy) } } -type graphNodeCreatorTest struct { - AddrString string - Refs []string -} +func testDestroyNode(addrString string) GraphNodeDestroyer { + instAddr := mustResourceInstanceAddr(addrString) -var ( - _ GraphNodeCreator = (*graphNodeCreatorTest)(nil) - _ GraphNodeReferencer = (*graphNodeCreatorTest)(nil) -) + abs := NewNodeAbstractResource(instAddr.ContainingResource()) -func (n *graphNodeCreatorTest) Name() string { - return n.CreateAddr().String() -} - -func (n *graphNodeCreatorTest) mustAddr() addrs.AbsResourceInstance { - addr, diags := addrs.ParseAbsResourceInstanceStr(n.AddrString) - if diags.HasErrors() { - panic(diags.Err()) - } - return addr -} - -func (n *graphNodeCreatorTest) Path() addrs.ModuleInstance { - return n.mustAddr().Module -} - -func (n *graphNodeCreatorTest) CreateAddr() *addrs.AbsResourceInstance { - addr := n.mustAddr() - return &addr -} - -func (n *graphNodeCreatorTest) References() []*addrs.Reference { - ret := make([]*addrs.Reference, len(n.Refs)) - for i, str := range n.Refs { - ref, diags := addrs.ParseRefStr(str) - if diags.HasErrors() { - panic(diags.Err()) - } - ret[i] = ref + inst := &NodeAbstractResourceInstance{ + NodeAbstractResource: *abs, + InstanceKey: instAddr.Resource.Key, } - return ret -} - -type graphNodeDestroyerTest struct { - AddrString string - CBD bool - Modified bool -} - -var _ GraphNodeDestroyer = (*graphNodeDestroyerTest)(nil) - -func (n *graphNodeDestroyerTest) Name() string { - result := n.DestroyAddr().String() + " (destroy)" - if n.Modified { - result += " (modified)" - } - - return result -} - -func (n *graphNodeDestroyerTest) mustAddr() addrs.AbsResourceInstance { - addr, diags := addrs.ParseAbsResourceInstanceStr(n.AddrString) - if diags.HasErrors() { - panic(diags.Err()) - } - return addr -} - -func (n *graphNodeDestroyerTest) CreateBeforeDestroy() bool { - return n.CBD -} - -func (n *graphNodeDestroyerTest) ModifyCreateBeforeDestroy(v bool) error { - n.Modified = true - return nil -} -func (n *graphNodeDestroyerTest) DestroyAddr() *addrs.AbsResourceInstance { - addr := n.mustAddr() - return &addr + return &NodeDestroyResourceInstance{NodeAbstractResourceInstance: inst} } const testTransformDestroyEdgeBasicStr = ` From 4a41af08b802bf961f8634b3ed87da41cb2e8981 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 16:04:16 -0500 Subject: [PATCH 07/17] new deps are more precise --- terraform/transform_destroy_cbd_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/terraform/transform_destroy_cbd_test.go b/terraform/transform_destroy_cbd_test.go index deaddf96c3d6..0831d7ad9728 100644 --- a/terraform/transform_destroy_cbd_test.go +++ b/terraform/transform_destroy_cbd_test.go @@ -341,12 +341,10 @@ func TestCBDEdgeTransformer_depNonCBDCountBoth(t *testing.T) { test_object.A\[0\] test_object.A\[0\] \(destroy deposed \w+\) test_object.A\[0\] - test_object.A\[1\] test_object.B\[0\] test_object.B\[1\] test_object.A\[1\] test_object.A\[1\] \(destroy deposed \w+\) - test_object.A\[0\] test_object.A\[1\] test_object.B\[0\] test_object.B\[1\] From a0ba481cad15f6a4764678266102886e63fd7730 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 16:04:25 -0500 Subject: [PATCH 08/17] add state where it's now needed for tests --- terraform/graph_builder_apply_test.go | 74 +++++++++++++++------------ 1 file changed, 41 insertions(+), 33 deletions(-) diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 593c6149a3e3..388db968a07f 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -235,12 +235,6 @@ func TestApplyGraphBuilder_doubleCBD(t *testing.T) { "test_object.B", destroyB, ) - - // actual := strings.TrimSpace(g.String()) - // expected := strings.TrimSpace(testApplyGraphBuilderDoubleCBDStr) - // if actual != expected { - // t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) - // } } // This tests the ordering of two resources being destroyed that depend @@ -263,33 +257,26 @@ func TestApplyGraphBuilder_destroyStateOnly(t *testing.T) { }, } - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "test_object.A": &ResourceState{ - Type: "test_object", - Primary: &InstanceState{ - ID: "foo", - Attributes: map[string]string{}, - }, - Provider: "provider.test", - }, - - "test_object.B": &ResourceState{ - Type: "test_object", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{}, - }, - Dependencies: []string{"test_object.A"}, - Provider: "provider.test", - }, - }, - }, + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), }, - }) + mustProviderConfig("provider.test"), + ) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) b := &ApplyGraphBuilder{ Config: testModule(t, "empty"), @@ -304,7 +291,6 @@ func TestApplyGraphBuilder_destroyStateOnly(t *testing.T) { if diags.HasErrors() { t.Fatalf("err: %s", diags.Err()) } - t.Logf("Graph:\n%s", g.String()) if g.Path.String() != addrs.RootModuleInstance.String() { t.Fatalf("wrong path %q", g.Path.String()) @@ -376,11 +362,33 @@ func TestApplyGraphBuilder_moduleDestroy(t *testing.T) { }, } + state := states.NewState() + modA := state.EnsureModule(addrs.RootModuleInstance.Child("A", addrs.NoKey)) + modA.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), + }, + mustProviderConfig("provider.test"), + ) + modB := state.EnsureModule(addrs.RootModuleInstance.Child("B", addrs.NoKey)) + modB.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo","value":"foo"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.A.test_object.foo")}, + }, + mustProviderConfig("provider.test"), + ) + b := &ApplyGraphBuilder{ Config: testModule(t, "graph-builder-apply-module-destroy"), Changes: changes, Components: simpleMockComponentFactory(), Schemas: simpleTestSchemas(), + State: state, } g, err := b.Build(addrs.RootModuleInstance) From 9edb719aaa48dfcf0a81711099cb41de1a8b20eb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 17:13:08 -0500 Subject: [PATCH 09/17] run AttachStateTransformer in destroy plan The AttachStateTransformer was never run in the destroy plan. This means that resource without configuration that used a non-default provider would not be connected to the correct provider for the plan. The test that was attempting to catch this only worked because the temporary graph used in the DestroyEdgeTransformer would add the state and detect some issues. --- terraform/context_apply_test.go | 43 ++++++++----------------- terraform/graph_builder_destroy_plan.go | 3 ++ 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 8f316c5fa1a4..0f544d1e39f9 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -10236,29 +10236,16 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { p.ApplyFn = testApplyFn p.DiffFn = testDiffFn - s := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - }, - &ModuleState{ - Path: []string{"root", "child"}, - }, - &ModuleState{ - Path: []string{"root", "mod", "removed"}, - Resources: map[string]*ResourceState{ - "aws_instance.child": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - // this provider doesn't exist - Provider: "provider.aws.baz", - }, - }, - }, + state := states.NewState() + removed := state.EnsureModule(addrs.RootModuleInstance.Child("mod", addrs.NoKey).Child("removed", addrs.NoKey)) + removed.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.child").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar"}`), }, - }) + mustProviderConfig("provider.aws.baz"), + ) ctx := testContext2(t, &ContextOpts{ Config: m, @@ -10267,7 +10254,7 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), }, ), - State: s, + State: state, Destroy: true, }) @@ -10277,12 +10264,10 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { } // correct the state - s.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.AbsProviderConfig{ - Provider: addrs.NewLegacyProvider("aws"), - Module: addrs.RootModuleInstance, - Alias: "bar", - } - + state.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.ProviderConfig{ + Type: addrs.NewLegacyProvider("aws"), + Alias: "bar", + }.Absolute(addrs.RootModuleInstance) if _, diags := ctx.Plan(); diags.HasErrors() { t.Fatal(diags.Err()) } diff --git a/terraform/graph_builder_destroy_plan.go b/terraform/graph_builder_destroy_plan.go index a6047a9b418c..08ee4e63e195 100644 --- a/terraform/graph_builder_destroy_plan.go +++ b/terraform/graph_builder_destroy_plan.go @@ -72,6 +72,9 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer { State: b.State, }, + // Attach the state + &AttachStateTransformer{State: b.State}, + // Attach the configuration to any resources &AttachResourceConfigTransformer{Config: b.Config}, From a4bc91abeb982e3e4a6e712cd61f0c35b2cda768 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 22:54:30 -0500 Subject: [PATCH 10/17] remove invalid destroy provisioner tests Remove all the destroy provisioner tests that are testing what is no longer allowed. Add missing state dependencies to remaining tests that require it. --- terraform/context_apply_test.go | 766 +++--------------- .../apply-provisioner-destroy-locals/main.tf | 14 - .../child/main.tf | 10 - .../apply-provisioner-destroy-module/main.tf | 4 - .../main.tf | 26 - .../apply-provisioner-destroy-outputs/main.tf | 23 - .../mod/main.tf | 5 - .../mod2/main.tf | 10 - .../main.tf | 12 - .../apply-provisioner-destroy-ref/main.tf | 12 - .../testdata/apply-provisioner-each/main.tf | 2 +- 11 files changed, 98 insertions(+), 786 deletions(-) delete mode 100644 terraform/testdata/apply-provisioner-destroy-locals/main.tf delete mode 100644 terraform/testdata/apply-provisioner-destroy-module/child/main.tf delete mode 100644 terraform/testdata/apply-provisioner-destroy-module/main.tf delete mode 100644 terraform/testdata/apply-provisioner-destroy-multiple-locals/main.tf delete mode 100644 terraform/testdata/apply-provisioner-destroy-outputs/main.tf delete mode 100644 terraform/testdata/apply-provisioner-destroy-outputs/mod/main.tf delete mode 100644 terraform/testdata/apply-provisioner-destroy-outputs/mod2/main.tf delete mode 100644 terraform/testdata/apply-provisioner-destroy-ref-invalid/main.tf delete mode 100644 terraform/testdata/apply-provisioner-destroy-ref/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 0f544d1e39f9..d311f00630bb 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -290,35 +290,26 @@ func TestContext2Apply_resourceDependsOnModuleStateOnly(t *testing.T) { p := testProvider("aws") p.DiffFn = testDiffFn - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.a": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "parent", - }, - Dependencies: []string{"module.child"}, - Provider: "provider.aws", - }, - }, - }, - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "aws_instance.child": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "child", - }, - Provider: "provider.aws", - }, - }, - }, + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.a").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"parent"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.aws_instance.child")}, }, - }) + mustProviderConfig("provider.aws"), + ) + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.child").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"child"}`), + }, + mustProviderConfig("provider.aws"), + ) { // verify the apply happens in the correct order @@ -1264,30 +1255,26 @@ func testContext2Apply_destroyDependsOn(t *testing.T) { p := testProvider("aws") p.ApplyFn = testApplyFn p.DiffFn = testDiffFn - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "foo", - Attributes: map[string]string{}, - }, - }, - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{}, - }, - }, - }, - }, + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.bar").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar"}`), }, - }) + mustProviderConfig("provider.aws"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("aws_instance.bar")}, + }, + mustProviderConfig("provider.aws"), + ) // Record the order we see Apply var actual []string @@ -1329,34 +1316,6 @@ func testContext2Apply_destroyDependsOn(t *testing.T) { // Test that destroy ordering is correct with dependencies only // in the state. func TestContext2Apply_destroyDependsOnStateOnly(t *testing.T) { - legacyState := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "foo", - Attributes: map[string]string{}, - }, - Provider: "provider.aws", - }, - - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{}, - }, - Dependencies: []string{"aws_instance.foo"}, - Provider: "provider.aws", - }, - }, - }, - }, - }) - newState := states.NewState() root := newState.EnsureModule(addrs.RootModuleInstance) root.SetResourceInstanceCurrent( @@ -1404,9 +1363,6 @@ func TestContext2Apply_destroyDependsOnStateOnly(t *testing.T) { // It is possible for this to be racy, so we loop a number of times // just to check. for i := 0; i < 10; i++ { - t.Run("legacy", func(t *testing.T) { - testContext2Apply_destroyDependsOnStateOnly(t, legacyState) - }) t.Run("new", func(t *testing.T) { testContext2Apply_destroyDependsOnStateOnly(t, newState) }) @@ -1458,34 +1414,6 @@ func testContext2Apply_destroyDependsOnStateOnly(t *testing.T, state *states.Sta // Test that destroy ordering is correct with dependencies only // in the state within a module (GH-11749) func TestContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { - legacyState := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "foo", - Attributes: map[string]string{}, - }, - Provider: "provider.aws", - }, - - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{}, - }, - Dependencies: []string{"aws_instance.foo"}, - Provider: "provider.aws", - }, - }, - }, - }, - }) - newState := states.NewState() child := newState.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) child.SetResourceInstanceCurrent( @@ -1533,9 +1461,6 @@ func TestContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { // It is possible for this to be racy, so we loop a number of times // just to check. for i := 0; i < 10; i++ { - t.Run("legacy", func(t *testing.T) { - testContext2Apply_destroyDependsOnStateOnlyModule(t, legacyState) - }) t.Run("new", func(t *testing.T) { testContext2Apply_destroyDependsOnStateOnlyModule(t, newState) }) @@ -2118,72 +2043,6 @@ aws_instance.foo: `) } -// for_each values cannot be used in the provisioner during destroy. -// There may be a way to handle this, but for now make sure we print an error -// rather than crashing with an invalid config. -func TestContext2Apply_provisionerDestroyForEach(t *testing.T) { - m := testModule(t, "apply-provisioner-each") - p := testProvider("aws") - pr := testProvisioner() - p.DiffFn = testDiffFn - p.ApplyFn = testApplyFn - - s := &states.State{ - Modules: map[string]*states.Module{ - "": &states.Module{ - Resources: map[string]*states.Resource{ - "aws_instance.bar": &states.Resource{ - Addr: addrs.Resource{Mode: 77, Type: "aws_instance", Name: "bar"}, - EachMode: states.EachMap, - Instances: map[addrs.InstanceKey]*states.ResourceInstance{ - addrs.StringKey("a"): &states.ResourceInstance{ - Current: &states.ResourceInstanceObjectSrc{ - AttrsJSON: []byte(`{"foo":"bar","id":"foo"}`), - }, - }, - addrs.StringKey("b"): &states.ResourceInstance{ - Current: &states.ResourceInstanceObjectSrc{ - AttrsJSON: []byte(`{"foo":"bar","id":"foo"}`), - }, - }, - }, - ProviderConfig: addrs.AbsProviderConfig{ - Module: addrs.ModuleInstance(nil), - Provider: addrs.NewLegacyProvider("aws"), - }, - }, - }, - }, - }, - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - State: s, - Destroy: true, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatalf("plan errors: %s", diags.Err()) - } - - _, diags := ctx.Apply() - if diags == nil { - t.Fatal("should error") - } - if !strings.Contains(diags.Err().Error(), "each.value cannot be used in this context") { - t.Fatal("unexpected error:", diags.Err()) - } -} - func TestContext2Apply_cancelProvisioner(t *testing.T) { m := testModule(t, "apply-cancel-provisioner") p := testProvider("aws") @@ -2831,30 +2690,26 @@ func TestContext2Apply_moduleDestroyOrder(t *testing.T) { }, } - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.b": resourceState("aws_instance", "b"), - }, - }, - - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "aws_instance.a": resourceState("aws_instance", "a"), - }, - Outputs: map[string]*OutputState{ - "a_output": &OutputState{ - Type: "string", - Sensitive: false, - Value: "a", - }, - }, - }, + state := states.NewState() + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.a").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a"}`), }, - }) + mustProviderConfig("provider.aws"), + ) + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.b").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.aws_instance.a")}, + }, + mustProviderConfig("provider.aws"), + ) ctx := testContext2(t, &ContextOpts{ Config: m, @@ -5763,41 +5618,24 @@ aws_instance.foo: } } -func TestContext2Apply_provisionerDestroyModule(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-module") +func TestContext2Apply_provisionerResourceRef(t *testing.T) { + m := testModule(t, "apply-provisioner-resource-ref") p := testProvider("aws") - pr := testProvisioner() p.ApplyFn = testApplyFn p.DiffFn = testDiffFn + + pr := testProvisioner() pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error { val, ok := c.Config["command"] - if !ok || val != "value" { + if !ok || val != "2" { t.Fatalf("bad value for foo: %v %#v", val, c) } return nil } - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - }, - }, - }, - }, - }) - ctx := testContext2(t, &ContextOpts{ - Config: m, - State: state, - Destroy: true, + Config: m, ProviderResolver: providers.ResolverFixed( map[addrs.Provider]providers.Factory{ addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), @@ -5817,7 +5655,11 @@ func TestContext2Apply_provisionerDestroyModule(t *testing.T) { t.Fatalf("diags: %s", diags.Err()) } - checkStateString(t, state, ``) + actual := strings.TrimSpace(state.String()) + expected := strings.TrimSpace(testTerraformApplyProvisionerResourceRefStr) + if actual != expected { + t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) + } // Verify apply was invoked if !pr.ProvisionResourceCalled { @@ -5825,53 +5667,23 @@ func TestContext2Apply_provisionerDestroyModule(t *testing.T) { } } -func TestContext2Apply_provisionerDestroyRef(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-ref") +func TestContext2Apply_provisionerSelfRef(t *testing.T) { + m := testModule(t, "apply-provisioner-self-ref") p := testProvider("aws") pr := testProvisioner() p.ApplyFn = testApplyFn p.DiffFn = testDiffFn pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error { val, ok := c.Config["command"] - if !ok || val != "hello" { - return fmt.Errorf("bad value for command: %v %#v", val, c) + if !ok || val != "bar" { + t.Fatalf("bad value for command: %v %#v", val, c) } return nil } - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{ - "value": "hello", - }, - }, - Provider: "provider.aws", - }, - - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - Provider: "provider.aws", - }, - }, - }, - }, - }) - ctx := testContext2(t, &ContextOpts{ - Config: m, - State: state, - Destroy: true, + Config: m, ProviderResolver: providers.ResolverFixed( map[addrs.Provider]providers.Factory{ addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), @@ -5891,158 +5703,11 @@ func TestContext2Apply_provisionerDestroyRef(t *testing.T) { t.Fatalf("diags: %s", diags.Err()) } - checkStateString(t, state, ``) - - // Verify apply was invoked - if !pr.ProvisionResourceCalled { - t.Fatalf("provisioner not invoked") - } -} - -// Test that a destroy provisioner referencing an invalid key errors. -func TestContext2Apply_provisionerDestroyRefInvalid(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-ref-invalid") - p := testProvider("aws") - pr := testProvisioner() - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error { - return nil - } - - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - }, - - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - }, - }, - }, - }, - }) - - ctx := testContext2(t, &ContextOpts{ - Config: m, - State: state, - Destroy: true, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - }) - - // this was an apply test, but this is now caught in Validation - if diags := ctx.Validate(); !diags.HasErrors() { - t.Fatal("expected error") - } -} - -func TestContext2Apply_provisionerResourceRef(t *testing.T) { - m := testModule(t, "apply-provisioner-resource-ref") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - - pr := testProvisioner() - pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error { - val, ok := c.Config["command"] - if !ok || val != "2" { - t.Fatalf("bad value for foo: %v %#v", val, c) - } - - return nil - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatalf("plan errors: %s", diags.Err()) - } - - state, diags := ctx.Apply() - if diags.HasErrors() { - t.Fatalf("diags: %s", diags.Err()) - } - - actual := strings.TrimSpace(state.String()) - expected := strings.TrimSpace(testTerraformApplyProvisionerResourceRefStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) - } - - // Verify apply was invoked - if !pr.ProvisionResourceCalled { - t.Fatalf("provisioner not invoked") - } -} - -func TestContext2Apply_provisionerSelfRef(t *testing.T) { - m := testModule(t, "apply-provisioner-self-ref") - p := testProvider("aws") - pr := testProvisioner() - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error { - val, ok := c.Config["command"] - if !ok || val != "bar" { - t.Fatalf("bad value for command: %v %#v", val, c) - } - - return nil - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatalf("plan errors: %s", diags.Err()) - } - - state, diags := ctx.Apply() - if diags.HasErrors() { - t.Fatalf("diags: %s", diags.Err()) - } - - actual := strings.TrimSpace(state.String()) - expected := strings.TrimSpace(testTerraformApplyProvisionerSelfRefStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) - } + actual := strings.TrimSpace(state.String()) + expected := strings.TrimSpace(testTerraformApplyProvisionerSelfRefStr) + if actual != expected { + t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) + } // Verify apply was invoked if !pr.ProvisionResourceCalled { @@ -8350,259 +8015,32 @@ aws_instance.bar: `) } -func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-locals") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - - pr := testProvisioner() - pr.ApplyFn = func(_ *InstanceState, rc *ResourceConfig) error { - cmd, ok := rc.Get("command") - if !ok || cmd != "local" { - return fmt.Errorf("provisioner got %v:%s", ok, cmd) - } - return nil - } - pr.GetSchemaResponse = provisioners.GetSchemaResponse{ - Provisioner: &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "command": { - Type: cty.String, - Required: true, - }, - "when": { - Type: cty.String, - Optional: true, - }, - }, - }, - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - State: MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": resourceState("aws_instance", "1234"), - }, - }, - }, - }), - Destroy: true, - // the test works without targeting, but this also tests that the local - // node isn't inadvertently pruned because of the wrong evaluation - // order. - Targets: []addrs.Targetable{ - addrs.RootModuleInstance.Resource( - addrs.ManagedResourceMode, "aws_instance", "foo", - ), - }, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - if _, diags := ctx.Apply(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - if !pr.ProvisionResourceCalled { - t.Fatal("provisioner not called") - } -} - -// this also tests a local value in the config referencing a resource that -// wasn't in the state during destroy. -func TestContext2Apply_destroyProvisionerWithMultipleLocals(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-multiple-locals") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - - pr := testProvisioner() - pr.GetSchemaResponse = provisioners.GetSchemaResponse{ - Provisioner: &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "id": { - Type: cty.String, - Required: true, - }, - "command": { - Type: cty.String, - Required: true, - }, - "when": { - Type: cty.String, - Optional: true, - }, - }, - }, - } - - pr.ApplyFn = func(is *InstanceState, rc *ResourceConfig) error { - cmd, ok := rc.Get("command") - if !ok { - return errors.New("no command in provisioner") - } - id, ok := rc.Get("id") - if !ok { - return errors.New("no id in provisioner") - } - - switch id { - case "1234": - if cmd != "local" { - return fmt.Errorf("provisioner %q got:%q", is.ID, cmd) - } - case "3456": - if cmd != "1234" { - return fmt.Errorf("provisioner %q got:%q", is.ID, cmd) - } - default: - t.Fatal("unknown instance") - } - return nil - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - State: MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": resourceState("aws_instance", "1234"), - "aws_instance.bar": resourceState("aws_instance", "3456"), - }, - }, - }, - }), - Destroy: true, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - if _, diags := ctx.Apply(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - if !pr.ProvisionResourceCalled { - t.Fatal("provisioner not called") - } -} - -func TestContext2Apply_destroyProvisionerWithOutput(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-outputs") +func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { + m := testModule(t, "apply-destroy-targeted-count") p := testProvider("aws") p.ApplyFn = testApplyFn p.DiffFn = testDiffFn - pr := testProvisioner() - pr.ApplyFn = func(is *InstanceState, rc *ResourceConfig) error { - cmd, ok := rc.Get("command") - if !ok || cmd != "3" { - return fmt.Errorf("provisioner for %s got %v:%s", is.ID, ok, cmd) - } - return nil - } - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"i-bcd345"}`), }, - State: MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": resourceState("aws_instance", "1"), - }, - Outputs: map[string]*OutputState{ - "value": { - Type: "string", - Value: "3", - }, - }, - }, - &ModuleState{ - Path: []string{"root", "mod"}, - Resources: map[string]*ResourceState{ - "aws_instance.baz": resourceState("aws_instance", "3"), - }, - // state needs to be properly initialized - Outputs: map[string]*OutputState{}, - }, - &ModuleState{ - Path: []string{"root", "mod2"}, - Resources: map[string]*ResourceState{ - "aws_instance.bar": resourceState("aws_instance", "2"), - }, - }, - }, - }), - Destroy: true, - - // targeting the source of the value used by all resources should still - // destroy them all. - Targets: []addrs.Targetable{ - addrs.RootModuleInstance.Child("mod", addrs.NoKey).Resource( - addrs.ManagedResourceMode, "aws_instance", "baz", - ), + mustProviderConfig("provider.aws"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.bar").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"i-abc123"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("aws_instance.foo")}, }, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - state, diags := ctx.Apply() - if diags.HasErrors() { - t.Fatal(diags.Err()) - } - if !pr.ProvisionResourceCalled { - t.Fatal("provisioner not called") - } - - // confirm all outputs were removed too - for _, mod := range state.Modules { - if len(mod.OutputValues) > 0 { - t.Fatalf("output left in module state: %#v\n", mod) - } - } -} + mustProviderConfig("provider.aws"), + ) -func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { - m := testModule(t, "apply-destroy-targeted-count") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn ctx := testContext2(t, &ContextOpts{ Config: m, ProviderResolver: providers.ResolverFixed( @@ -8610,17 +8048,7 @@ func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), }, ), - State: MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.foo": resourceState("aws_instance", "i-bcd345"), - "aws_instance.bar": resourceState("aws_instance", "i-abc123"), - }, - }, - }, - }), + State: state, Targets: []addrs.Targetable{ addrs.RootModuleInstance.Resource( addrs.ManagedResourceMode, "aws_instance", "foo", diff --git a/terraform/testdata/apply-provisioner-destroy-locals/main.tf b/terraform/testdata/apply-provisioner-destroy-locals/main.tf deleted file mode 100644 index 5818e7c7d53a..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-locals/main.tf +++ /dev/null @@ -1,14 +0,0 @@ -locals { - value = "local" -} - -resource "aws_instance" "foo" { - provisioner "shell" { - command = "${local.value}" - when = "create" - } - provisioner "shell" { - command = "${local.value}" - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-destroy-module/child/main.tf b/terraform/testdata/apply-provisioner-destroy-module/child/main.tf deleted file mode 100644 index a8b8d123cccd..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-module/child/main.tf +++ /dev/null @@ -1,10 +0,0 @@ -variable "key" {} - -resource "aws_instance" "foo" { - foo = "bar" - - provisioner "shell" { - command = "${var.key}" - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-destroy-module/main.tf b/terraform/testdata/apply-provisioner-destroy-module/main.tf deleted file mode 100644 index 817ae043dfe9..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-module/main.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child" { - source = "./child" - key = "value" -} diff --git a/terraform/testdata/apply-provisioner-destroy-multiple-locals/main.tf b/terraform/testdata/apply-provisioner-destroy-multiple-locals/main.tf deleted file mode 100644 index 2050b19a121a..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-multiple-locals/main.tf +++ /dev/null @@ -1,26 +0,0 @@ -locals { - value = "local" - foo_id = aws_instance.foo.id - - // baz is not in the state during destroy, but this is a valid config that - // should not fail. - baz_id = aws_instance.baz.id -} - -resource "aws_instance" "baz" {} - -resource "aws_instance" "foo" { - provisioner "shell" { - id = self.id - command = local.value - when = "destroy" - } -} - -resource "aws_instance" "bar" { - provisioner "shell" { - id = self.id - command = local.foo_id - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-destroy-outputs/main.tf b/terraform/testdata/apply-provisioner-destroy-outputs/main.tf deleted file mode 100644 index 9a5843aa3cc9..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-outputs/main.tf +++ /dev/null @@ -1,23 +0,0 @@ -module "mod" { - source = "./mod" -} - -locals { - value = "${module.mod.value}" -} - -resource "aws_instance" "foo" { - provisioner "shell" { - command = "${local.value}" - when = "destroy" - } -} - -module "mod2" { - source = "./mod2" - value = "${module.mod.value}" -} - -output "value" { - value = "${local.value}" -} diff --git a/terraform/testdata/apply-provisioner-destroy-outputs/mod/main.tf b/terraform/testdata/apply-provisioner-destroy-outputs/mod/main.tf deleted file mode 100644 index 1f4ad09c5449..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-outputs/mod/main.tf +++ /dev/null @@ -1,5 +0,0 @@ -output "value" { - value = "${aws_instance.baz.id}" -} - -resource "aws_instance" "baz" {} diff --git a/terraform/testdata/apply-provisioner-destroy-outputs/mod2/main.tf b/terraform/testdata/apply-provisioner-destroy-outputs/mod2/main.tf deleted file mode 100644 index a476bb9207c1..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-outputs/mod2/main.tf +++ /dev/null @@ -1,10 +0,0 @@ -variable "value" { -} - -resource "aws_instance" "bar" { - provisioner "shell" { - command = "${var.value}" - when = "destroy" - } -} - diff --git a/terraform/testdata/apply-provisioner-destroy-ref-invalid/main.tf b/terraform/testdata/apply-provisioner-destroy-ref-invalid/main.tf deleted file mode 100644 index fb4a96b100da..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-ref-invalid/main.tf +++ /dev/null @@ -1,12 +0,0 @@ -resource "aws_instance" "bar" { - value = "hello" -} - -resource "aws_instance" "foo" { - foo = "bar" - - provisioner "shell" { - command = aws_instance.bar.does_not_exist - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-destroy-ref/main.tf b/terraform/testdata/apply-provisioner-destroy-ref/main.tf deleted file mode 100644 index b36916df17f0..000000000000 --- a/terraform/testdata/apply-provisioner-destroy-ref/main.tf +++ /dev/null @@ -1,12 +0,0 @@ -resource "aws_instance" "bar" { - value = "hello" -} - -resource "aws_instance" "foo" { - foo = "bar" - - provisioner "shell" { - command = "${aws_instance.bar.value}" - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-each/main.tf b/terraform/testdata/apply-provisioner-each/main.tf index 8d29a5c167ce..29be7206eb57 100644 --- a/terraform/testdata/apply-provisioner-each/main.tf +++ b/terraform/testdata/apply-provisioner-each/main.tf @@ -2,6 +2,6 @@ resource "aws_instance" "bar" { for_each = toset(["a"]) provisioner "shell" { when = "destroy" - command = "echo ${each.value}" + command = "echo ${each.key}" } } From dc8cdd260cb59cf00b1588e1b7b68d691d075649 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 3 Dec 2019 23:20:18 -0500 Subject: [PATCH 11/17] add missing deps to targeted destroy test --- command/apply_destroy_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go index b3592a301b47..bdcfa1a074e4 100644 --- a/command/apply_destroy_test.go +++ b/command/apply_destroy_test.go @@ -195,7 +195,7 @@ func TestApply_destroyTargeted(t *testing.T) { Mode: addrs.ManagedResourceMode, Type: "test_instance", Name: "foo", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + }.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance), &states.ResourceInstanceObjectSrc{ AttrsJSON: []byte(`{"id":"i-ab123"}`), Status: states.ObjectReady, @@ -212,8 +212,9 @@ func TestApply_destroyTargeted(t *testing.T) { Name: "foo", }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), &states.ResourceInstanceObjectSrc{ - AttrsJSON: []byte(`{"id":"i-abc123"}`), - Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"i-abc123"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_instance.foo")}, + Status: states.ObjectReady, }, addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("test"), From ca5b0e689432d0f1c03eb72ecdfdcaddfbd6bdac Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 4 Dec 2019 09:36:48 -0500 Subject: [PATCH 12/17] no longer need DestroyValueReferenceTransformer since destroy nodes are no longer connected to values, there's no need to try and wrangle their edges to prevent cycles during destroy. --- terraform/graph_builder_apply.go | 6 +---- terraform/transform_destroy_edge.go | 2 -- terraform/transform_reference.go | 36 ----------------------------- 3 files changed, 1 insertion(+), 43 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 00ddcf5c4034..292e8414a78b 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -171,14 +171,10 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Destroy: b.Destroy, }, - // Handle destroy time transformations for output and local values. - // Reverse the edges from outputs and locals, so that - // interpolations don't fail during destroy. - // Create a destroy node for outputs to remove them from the state. GraphTransformIf( func() bool { return b.Destroy }, GraphTransformMulti( - &DestroyValueReferenceTransformer{}, + // Create a destroy node for outputs to remove them from the state. &DestroyOutputTransformer{}, ), ), diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index d954d2c048db..5092b3abbbb9 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -56,7 +56,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { // Build a map of what is being destroyed (by address string) to // the list of destroyers. destroyers := make(map[string][]GraphNodeDestroyer) - destroyerAddrs := make(map[string]addrs.AbsResourceInstance) // Record the creators, which will need to depend on the destroyers if they // are only being updated. @@ -79,7 +78,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { key := addr.String() log.Printf("[TRACE] DestroyEdgeTransformer: %q (%T) destroys %s", dag.VertexName(n), v, key) destroyers[key] = append(destroyers[key], n) - destroyerAddrs[key] = addr resAddr := addr.Resource.Resource.Absolute(addr.Module).String() destroyersByResource[resAddr] = append(destroyersByResource[resAddr], n) diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index ceff6914875b..371265bab977 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -176,42 +176,6 @@ func (t AttachDependenciesTransformer) Transform(g *Graph) error { return nil } -// DestroyReferenceTransformer is a GraphTransformer that reverses the edges -// for locals and outputs that depend on other nodes which will be -// removed during destroy. If a destroy node is evaluated before the local or -// output value, it will be removed from the state, and the later interpolation -// will fail. -type DestroyValueReferenceTransformer struct{} - -func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { - vs := g.Vertices() - for _, v := range vs { - switch v.(type) { - case *NodeApplyableOutput, *NodeLocal: - // OK - default: - continue - } - - // reverse any outgoing edges so that the value is evaluated first. - for _, e := range g.EdgesFrom(v) { - target := e.Target() - - // only destroy nodes will be evaluated in reverse - if _, ok := target.(GraphNodeDestroyer); !ok { - continue - } - - log.Printf("[TRACE] output dep: %s", dag.VertexName(target)) - - g.RemoveEdge(e) - g.Connect(dag.BasicEdge(target, v)) - } - } - - return nil -} - // PruneUnusedValuesTransformer is a GraphTransformer that removes local, // variable, and output values which are not referenced in the graph. If these // values reference a resource that is no longer in the state the interpolation From 8c5853ee4e4af1cda11aefcf54f5494ca5d3928a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 12 Dec 2019 16:36:17 -0500 Subject: [PATCH 13/17] remove old references code from abstract resource --- terraform/node_resource_abstract.go | 41 ----------------------------- 1 file changed, 41 deletions(-) diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index 2149f45354f0..bab23fd5e62b 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -10,7 +10,6 @@ import ( "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/lang" "github.com/hashicorp/terraform/states" - "github.com/hashicorp/terraform/tfdiags" ) // ConcreteResourceNodeFunc is a callback type used to convert an @@ -241,46 +240,6 @@ func (n *NodeAbstractResourceInstance) References() []*addrs.Reference { return n.NodeAbstractResource.References() } - // FIXME: remove once the deprecated DependsOn values have been removed from state - // The state dependencies are now connected in a separate transformation as - // absolute addresses, but we need to keep this here until we can be sure - // that no state will need to use the old depends_on references. - if rs := n.ResourceState; rs != nil { - if s := rs.Instance(n.InstanceKey); s != nil { - // State is still storing dependencies as old-style strings, so we'll - // need to do a little work here to massage this to the form we now - // want. - var result []*addrs.Reference - - // It is (apparently) possible for s.Current to be nil. This proved - // difficult to reproduce, so we will fix the symptom here and hope - // to find the root cause another time. - // - // https://github.com/hashicorp/terraform/issues/21407 - if s.Current == nil { - log.Printf("[WARN] no current state found for %s", n.Name()) - return nil - } - for _, addr := range s.Current.DependsOn { - if addr == nil { - // Should never happen; indicates a bug in the state loader - panic(fmt.Sprintf("dependencies for current object on %s contains nil address", n.ResourceInstanceAddr())) - } - - // This is a little weird: we need to manufacture an addrs.Reference - // with a fake range here because the state isn't something we can - // make source references into. - result = append(result, &addrs.Reference{ - Subject: addr, - SourceRange: tfdiags.SourceRange{ - Filename: "(state file)", - }, - }) - } - return result - } - } - // If we have neither config nor state then we have no references. return nil } From d4d99be2db58effee2a7b4f8fcafaeb36234dc35 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 12 Dec 2019 21:36:27 -0500 Subject: [PATCH 14/17] remove some destroy special cases We no longer need special cases for most things during a full destroy, so remove those from the graph transformations. The only remaining cases are: - remove the root outputs, so destroy ends up with a clean state - reverse the target deps when targeting a destroy. --- terraform/graph_builder_apply.go | 10 ++-------- terraform/transform_destroy_cbd.go | 7 ------- terraform/transform_output.go | 6 ++++++ 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 292e8414a78b..a9aba06bb918 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -168,16 +168,10 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Config: b.Config, State: b.State, Schemas: b.Schemas, - Destroy: b.Destroy, }, - GraphTransformIf( - func() bool { return b.Destroy }, - GraphTransformMulti( - // Create a destroy node for outputs to remove them from the state. - &DestroyOutputTransformer{}, - ), - ), + // Create a destroy node for outputs to remove them from the state. + &DestroyOutputTransformer{Destroy: b.Destroy}, // Prune unreferenced values, which may have interpolations that can't // be resolved. diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index 40474a0b4d50..0bfcda96eefa 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -134,16 +134,9 @@ type CBDEdgeTransformer struct { // obtain schema information from providers and provisioners so we can // properly resolve implicit dependencies. Schemas *Schemas - - // If the operation is a simple destroy, no transformation is done. - Destroy bool } func (t *CBDEdgeTransformer) Transform(g *Graph) error { - if t.Destroy { - return nil - } - // Go through and reverse any destroy edges for _, v := range g.Vertices() { dn, ok := v.(GraphNodeDestroyerCBD) diff --git a/terraform/transform_output.go b/terraform/transform_output.go index ed93cdb8732a..f68228b38469 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -60,9 +60,15 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error { // outputs during destroy. We need to do this to ensure that no stale outputs // are ever left in the state. type DestroyOutputTransformer struct { + Destroy bool } func (t *DestroyOutputTransformer) Transform(g *Graph) error { + // Only clean root outputs on a full destroy + if !t.Destroy { + return nil + } + for _, v := range g.Vertices() { output, ok := v.(*NodeApplyableOutput) if !ok { From 099806c128906492a4b086d3c9c98ad4343eca74 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 11 Feb 2020 19:10:41 -0500 Subject: [PATCH 15/17] fixup LocalProviderConfig literal --- terraform/context_apply_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index d311f00630bb..b700c4cd5fb1 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9692,9 +9692,9 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { } // correct the state - state.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.ProviderConfig{ - Type: addrs.NewLegacyProvider("aws"), - Alias: "bar", + state.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.LocalProviderConfig{ + LocalName: "aws", + Alias: "bar", }.Absolute(addrs.RootModuleInstance) if _, diags := ctx.Plan(); diags.HasErrors() { t.Fatal(diags.Err()) From b4f06c22fe2ec9300a35f90f76f82f82b401cb32 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 13 Feb 2020 16:05:28 -0500 Subject: [PATCH 16/17] fixup provider types in new tests --- terraform/context_apply_test.go | 28 +++++++++++++----------- terraform/graph_builder_apply_test.go | 8 +++---- terraform/transform_destroy_edge_test.go | 20 ++++++++--------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index b700c4cd5fb1..af5916fbb591 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -299,7 +299,7 @@ func TestContext2Apply_resourceDependsOnModuleStateOnly(t *testing.T) { AttrsJSON: []byte(`{"id":"parent"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.aws_instance.child")}, }, - mustProviderConfig("provider.aws"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) child.SetResourceInstanceCurrent( @@ -308,7 +308,7 @@ func TestContext2Apply_resourceDependsOnModuleStateOnly(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"child"}`), }, - mustProviderConfig("provider.aws"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) { @@ -1264,7 +1264,7 @@ func testContext2Apply_destroyDependsOn(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"bar"}`), }, - mustProviderConfig("provider.aws"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) root.SetResourceInstanceCurrent( mustResourceInstanceAddr("aws_instance.foo").Resource, @@ -1273,7 +1273,7 @@ func testContext2Apply_destroyDependsOn(t *testing.T) { AttrsJSON: []byte(`{"id":"foo"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("aws_instance.bar")}, }, - mustProviderConfig("provider.aws"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) // Record the order we see Apply @@ -2698,7 +2698,7 @@ func TestContext2Apply_moduleDestroyOrder(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"a"}`), }, - mustProviderConfig("provider.aws"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) root := state.EnsureModule(addrs.RootModuleInstance) root.SetResourceInstanceCurrent( @@ -2708,7 +2708,7 @@ func TestContext2Apply_moduleDestroyOrder(t *testing.T) { AttrsJSON: []byte(`{"id":"b"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.aws_instance.a")}, }, - mustProviderConfig("provider.aws"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) ctx := testContext2(t, &ContextOpts{ @@ -8029,7 +8029,7 @@ func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"i-bcd345"}`), }, - mustProviderConfig("provider.aws"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) root.SetResourceInstanceCurrent( mustResourceInstanceAddr("aws_instance.bar").Resource, @@ -8038,7 +8038,7 @@ func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { AttrsJSON: []byte(`{"id":"i-abc123"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("aws_instance.foo")}, }, - mustProviderConfig("provider.aws"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), ) ctx := testContext2(t, &ContextOpts{ @@ -9672,7 +9672,7 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"bar"}`), }, - mustProviderConfig("provider.aws.baz"), + mustProviderConfig(`provider["registry.terraform.io/-/aws"].baz`), ) ctx := testContext2(t, &ContextOpts{ @@ -9692,10 +9692,12 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { } // correct the state - state.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.LocalProviderConfig{ - LocalName: "aws", - Alias: "bar", - }.Absolute(addrs.RootModuleInstance) + state.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.AbsProviderConfig{ + Provider: addrs.NewLegacyProvider("aws"), + Alias: "bar", + Module: addrs.RootModuleInstance, + } + if _, diags := ctx.Plan(); diags.HasErrors() { t.Fatal(diags.Err()) } diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 388db968a07f..c0827d05b314 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -266,7 +266,7 @@ func TestApplyGraphBuilder_destroyStateOnly(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo"}`), }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) child.SetResourceInstanceCurrent( mustResourceInstanceAddr("test_object.B").Resource, @@ -275,7 +275,7 @@ func TestApplyGraphBuilder_destroyStateOnly(t *testing.T) { AttrsJSON: []byte(`{"id":"bar"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.A")}, }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) b := &ApplyGraphBuilder{ @@ -370,7 +370,7 @@ func TestApplyGraphBuilder_moduleDestroy(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo"}`), }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) modB := state.EnsureModule(addrs.RootModuleInstance.Child("B", addrs.NoKey)) modB.SetResourceInstanceCurrent( @@ -380,7 +380,7 @@ func TestApplyGraphBuilder_moduleDestroy(t *testing.T) { AttrsJSON: []byte(`{"id":"foo","value":"foo"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("module.A.test_object.foo")}, }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) b := &ApplyGraphBuilder{ diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index 9659efeabc6a..19a141b52f76 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/terraform/transform_destroy_edge_test.go @@ -21,7 +21,7 @@ func TestDestroyEdgeTransformer_basic(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"A"}`), }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) root.SetResourceInstanceCurrent( mustResourceInstanceAddr("test_object.B").Resource, @@ -30,7 +30,7 @@ func TestDestroyEdgeTransformer_basic(t *testing.T) { AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { t.Fatal(err) @@ -65,7 +65,7 @@ func TestDestroyEdgeTransformer_multi(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"A"}`), }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) root.SetResourceInstanceCurrent( mustResourceInstanceAddr("test_object.B").Resource, @@ -74,7 +74,7 @@ func TestDestroyEdgeTransformer_multi(t *testing.T) { AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) root.SetResourceInstanceCurrent( mustResourceInstanceAddr("test_object.C").Resource, @@ -86,7 +86,7 @@ func TestDestroyEdgeTransformer_multi(t *testing.T) { mustResourceAddr("test_object.B"), }, }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { @@ -140,7 +140,7 @@ func TestDestroyEdgeTransformer_module(t *testing.T) { AttrsJSON: []byte(`{"id":"a"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.b")}, }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) child.SetResourceInstanceCurrent( mustResourceInstanceAddr("test_object.b").Resource, @@ -148,7 +148,7 @@ func TestDestroyEdgeTransformer_module(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"b","test_string":"x"}`), }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { @@ -184,7 +184,7 @@ func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"a"}`), }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) child.SetResourceInstanceCurrent( mustResourceInstanceAddr("test_object.b").Resource, @@ -193,7 +193,7 @@ func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) { AttrsJSON: []byte(`{"id":"b","test_string":"x"}`), Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.a")}, }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) child.SetResourceInstanceCurrent( mustResourceInstanceAddr("test_object.c").Resource, @@ -205,7 +205,7 @@ func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) { mustResourceAddr("module.child.test_object.b"), }, }, - mustProviderConfig("provider.test"), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { From 37e60cd883581ff31bab49a984adc4bc91df1ea8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 13 Feb 2020 20:46:48 -0500 Subject: [PATCH 17/17] fix comment text --- configs/parser_config_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/configs/parser_config_test.go b/configs/parser_config_test.go index 2fe645ed7676..7832914c9b0c 100644 --- a/configs/parser_config_test.go +++ b/configs/parser_config_test.go @@ -226,10 +226,10 @@ func TestParserLoadConfigFileWarning(t *testing.T) { } // TestParseLoadConfigFileError is a test that verifies files from -// testdata/warning-files produce particular warnings. +// testdata/warning-files produce particular errors. // // This test does not verify that reading these files produces the correct -// file element contents in spite of those warnings. More detailed assertions +// file element contents in spite of those errors. More detailed assertions // may be made on some subset of these configuration files in other tests. func TestParserLoadConfigFileError(t *testing.T) { files, err := ioutil.ReadDir("testdata/error-files") @@ -247,7 +247,7 @@ func TestParserLoadConfigFileError(t *testing.T) { // First we'll scan the file to see what warnings are expected. // That's declared inside the files themselves by using the - // string "WARNING: " somewhere on each line that is expected + // string "ERROR: " somewhere on each line that is expected // to produce a warning, followed by the expected warning summary // text. A single-line comment (with #) is the main way to do that. const marker = "ERROR: "