From d15a324a534f339d662d0b0354993e3a0f03598a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 22 Jul 2022 10:20:17 -0400 Subject: [PATCH 1/4] Implement breadth-first walks and add tests Make DAG walks test-able, and add tests for more complex graph ordering. We also add breadth-first for comparison, though it's not used currently in Terraform. --- internal/dag/dag.go | 137 +++++++++++++++++++++++++-------------- internal/dag/dag_test.go | 119 ++++++++++++++++++++++++++-------- 2 files changed, 183 insertions(+), 73 deletions(-) diff --git a/internal/dag/dag.go b/internal/dag/dag.go index 6da10df51be9..fd086f684582 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -2,6 +2,7 @@ package dag import ( "fmt" + "sort" "strings" "github.com/hashicorp/terraform/internal/tfdiags" @@ -178,24 +179,72 @@ type vertexAtDepth struct { Depth int } +type walkType uint64 + +const ( + depthFirst walkType = 1 << iota + breadthFirst + downOrder + upOrder +) + // DepthFirstWalk does a depth-first walk of the graph starting from // the vertices in start. -// The algorithm used here does not do a complete topological sort. To ensure -// correct overall ordering run TransitiveReduction first. func (g *AcyclicGraph) DepthFirstWalk(start Set, f DepthWalkFunc) error { + return g.walk(depthFirst|downOrder, false, start, f) +} + +// ReverseDepthFirstWalk does a depth-first walk _up_ the graph starting from +// the vertices in start. +func (g *AcyclicGraph) ReverseDepthFirstWalk(start Set, f DepthWalkFunc) error { + return g.walk(depthFirst|upOrder, false, start, f) +} + +// BreadthFirstWalk does a breadth-first walk of the graph starting from +// the vertices in start. +func (g *AcyclicGraph) BreadthFirstWalk(start Set, f DepthWalkFunc) error { + return g.walk(breadthFirst|downOrder, false, start, f) +} + +// ReverseBreadthFirstWalk does a breadth-first walk _up_ the graph starting from +// the vertices in start. +func (g *AcyclicGraph) ReverseBreadthFirstWalk(start Set, f DepthWalkFunc) error { + return g.walk(breadthFirst|upOrder, false, start, f) +} + +// Setting test to true will walk sets of vertices in sorted order for +// deterministic testing. +func (g *AcyclicGraph) walk(order walkType, test bool, start Set, f DepthWalkFunc) error { seen := make(map[Vertex]struct{}) - frontier := make([]*vertexAtDepth, 0, len(start)) + frontier := make([]vertexAtDepth, 0, len(start)) for _, v := range start { - frontier = append(frontier, &vertexAtDepth{ + frontier = append(frontier, vertexAtDepth{ Vertex: v, Depth: 0, }) } + + if test { + testSortFrontier(frontier) + } + for len(frontier) > 0 { // Pop the current vertex - n := len(frontier) - current := frontier[n-1] - frontier = frontier[:n-1] + var current vertexAtDepth + + switch { + case order&depthFirst != 0: + // depth first, the frontier is used like a stack + n := len(frontier) + current = frontier[n-1] + frontier = frontier[:n-1] + case order&breadthFirst != 0: + // breadth first, the frontier is used like a queue + current = frontier[0] + frontier = frontier[1:] + default: + panic(fmt.Sprint("invalid visit order", order)) + } // Check if we've seen this already and return... if _, ok := seen[current.Vertex]; ok { @@ -208,54 +257,48 @@ func (g *AcyclicGraph) DepthFirstWalk(start Set, f DepthWalkFunc) error { return err } - for _, v := range g.downEdgesNoCopy(current.Vertex) { - frontier = append(frontier, &vertexAtDepth{ - Vertex: v, - Depth: current.Depth + 1, - }) + var edges Set + switch { + case order&downOrder != 0: + edges = g.downEdgesNoCopy(current.Vertex) + case order&upOrder != 0: + edges = g.upEdgesNoCopy(current.Vertex) + default: + panic(fmt.Sprint("invalid walk order", order)) } - } + if test { + frontier = testAppendNextSorted(frontier, edges, current.Depth+1) + } else { + frontier = appendNext(frontier, edges, current.Depth+1) + } + } return nil } -// ReverseDepthFirstWalk does a depth-first walk _up_ the graph starting from -// the vertices in start. -// The algorithm used here does not do a complete topological sort. To ensure -// correct overall ordering run TransitiveReduction first. -func (g *AcyclicGraph) ReverseDepthFirstWalk(start Set, f DepthWalkFunc) error { - seen := make(map[Vertex]struct{}) - frontier := make([]*vertexAtDepth, 0, len(start)) - for _, v := range start { - frontier = append(frontier, &vertexAtDepth{ +func appendNext(frontier []vertexAtDepth, next Set, depth int) []vertexAtDepth { + for _, v := range next { + frontier = append(frontier, vertexAtDepth{ Vertex: v, - Depth: 0, + Depth: depth, }) } - for len(frontier) > 0 { - // Pop the current vertex - n := len(frontier) - current := frontier[n-1] - frontier = frontier[:n-1] - - // Check if we've seen this already and return... - if _, ok := seen[current.Vertex]; ok { - continue - } - seen[current.Vertex] = struct{}{} - - for _, t := range g.upEdgesNoCopy(current.Vertex) { - frontier = append(frontier, &vertexAtDepth{ - Vertex: t, - Depth: current.Depth + 1, - }) - } + return frontier +} - // Visit the current node - if err := f(current.Vertex, current.Depth); err != nil { - return err - } +func testAppendNextSorted(frontier []vertexAtDepth, edges Set, depth int) []vertexAtDepth { + var newEdges []vertexAtDepth + for _, v := range edges { + newEdges = append(newEdges, vertexAtDepth{ + Vertex: v, + Depth: depth, + }) } - - return nil + testSortFrontier(newEdges) + return append(frontier, newEdges...) +} +func testSortFrontier(f []vertexAtDepth) { + sort.Slice(f, func(i, j int) bool { + return VertexName(f[i].Vertex) < VertexName(f[j].Vertex) + }) } diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 75cfb86ff0e0..6e9cf5ac4388 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -414,34 +414,101 @@ func BenchmarkDAG(b *testing.B) { } } -func TestAcyclicGraph_ReverseDepthFirstWalk_WithRemoval(t *testing.T) { - var g AcyclicGraph - g.Add(1) - g.Add(2) - g.Add(3) - g.Connect(BasicEdge(3, 2)) - g.Connect(BasicEdge(2, 1)) +func TestAcyclicGraphWalkOrder(t *testing.T) { + /* Sample dependency graph, + all edges pointing downwards. + 1 2 + / \ / \ + 3 4 5 + / \ / + 6 7 + / | \ + 8 9 10 + \ | / + 11 + */ - var visits []Vertex - var lock sync.Mutex - root := make(Set) - root.Add(1) - - err := g.ReverseDepthFirstWalk(root, func(v Vertex, d int) error { - lock.Lock() - defer lock.Unlock() - visits = append(visits, v) - g.Remove(v) - return nil - }) - if err != nil { - t.Fatalf("err: %s", err) - } - - expected := []Vertex{1, 2, 3} - if !reflect.DeepEqual(visits, expected) { - t.Fatalf("expected: %#v, got: %#v", expected, visits) + var g AcyclicGraph + for i := 0; i <= 11; i++ { + g.Add(i) } + g.Connect(BasicEdge(1, 3)) + g.Connect(BasicEdge(1, 4)) + g.Connect(BasicEdge(2, 4)) + g.Connect(BasicEdge(2, 5)) + g.Connect(BasicEdge(3, 6)) + g.Connect(BasicEdge(4, 7)) + g.Connect(BasicEdge(5, 7)) + g.Connect(BasicEdge(7, 8)) + g.Connect(BasicEdge(7, 9)) + g.Connect(BasicEdge(7, 10)) + g.Connect(BasicEdge(8, 11)) + g.Connect(BasicEdge(9, 11)) + g.Connect(BasicEdge(10, 11)) + + start := make(Set) + start.Add(2) + start.Add(1) + reverse := make(Set) + reverse.Add(11) + reverse.Add(6) + + t.Run("DepthFirst", func(t *testing.T) { + var visits []vertexAtDepth + g.walk(depthFirst|downOrder, true, start, func(v Vertex, d int) error { + visits = append(visits, vertexAtDepth{v, d}) + return nil + + }) + expect := []vertexAtDepth{ + {2, 0}, {5, 1}, {7, 2}, {9, 3}, {11, 4}, {8, 3}, {10, 3}, {4, 1}, {1, 0}, {3, 1}, {6, 2}, + } + if !reflect.DeepEqual(visits, expect) { + t.Errorf("expected visits:\n%v\ngot:\n%v\n", expect, visits) + } + }) + t.Run("ReverseDepthFirst", func(t *testing.T) { + var visits []vertexAtDepth + g.walk(depthFirst|upOrder, true, reverse, func(v Vertex, d int) error { + visits = append(visits, vertexAtDepth{v, d}) + return nil + + }) + expect := []vertexAtDepth{ + {6, 0}, {3, 1}, {1, 2}, {11, 0}, {9, 1}, {7, 2}, {5, 3}, {2, 4}, {4, 3}, {8, 1}, {10, 1}, + } + if !reflect.DeepEqual(visits, expect) { + t.Errorf("expected visits:\n%v\ngot:\n%v\n", expect, visits) + } + }) + t.Run("BreadthFirst", func(t *testing.T) { + var visits []vertexAtDepth + g.walk(breadthFirst|downOrder, true, start, func(v Vertex, d int) error { + visits = append(visits, vertexAtDepth{v, d}) + return nil + + }) + expect := []vertexAtDepth{ + {1, 0}, {2, 0}, {3, 1}, {4, 1}, {5, 1}, {6, 2}, {7, 2}, {10, 3}, {8, 3}, {9, 3}, {11, 4}, + } + if !reflect.DeepEqual(visits, expect) { + t.Errorf("expected visits:\n%v\ngot:\n%v\n", expect, visits) + } + }) + t.Run("ReverseBreadthFirst", func(t *testing.T) { + var visits []vertexAtDepth + g.walk(breadthFirst|upOrder, true, reverse, func(v Vertex, d int) error { + visits = append(visits, vertexAtDepth{v, d}) + return nil + + }) + expect := []vertexAtDepth{ + {11, 0}, {6, 0}, {10, 1}, {8, 1}, {9, 1}, {3, 1}, {7, 2}, {1, 2}, {4, 3}, {5, 3}, {2, 4}, + } + if !reflect.DeepEqual(visits, expect) { + t.Errorf("expected visits:\n%v\ngot:\n%v\n", expect, visits) + } + }) } const testGraphTransReductionStr = ` From a2c4ed3ad4f51e0655c738e105becc4563a5b1b1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 22 Jul 2022 12:04:50 -0400 Subject: [PATCH 2/4] Add methods for topological sorts A topological walk was previously only done in Terraform via the concurrent method used for walking the primary dependency graph in core. Sometime however we want a dependency ordering without the overhead of instantiating the concurrent walk with the channel-based edges. Add TopologicalOrder and ReverseTopologicalOrder to obtain a list of nodes which can be used to visit each while ensuring that all dependencies are satisfied. --- internal/dag/dag.go | 63 ++++++++++++++++++++++++++++++++++++++++ internal/dag/dag_test.go | 37 ++++++++++++++++++++++- 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/internal/dag/dag.go b/internal/dag/dag.go index fd086f684582..f5268e76f0c3 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -179,6 +179,69 @@ type vertexAtDepth struct { Depth int } +// TopologicalOrder returns a topological sort of the given graph. The nodes +// are not sorted, and any valid order may be returned. This function will +// panic if it encounters a cycle. +func (g *AcyclicGraph) TopologicalOrder() []Vertex { + return g.topoOrder(upOrder) +} + +// ReverseTopologicalOrder returns a topological sort of the given graph, +// following each edge in reverse. The nodes are not sorted, and any valid +// order may be returned. This function will panic if it encounters a cycle. +func (g *AcyclicGraph) ReverseTopologicalOrder() []Vertex { + return g.topoOrder(downOrder) +} + +func (g *AcyclicGraph) topoOrder(order walkType) []Vertex { + // Use a dfs-based sorting algorithm, similar to that used in + // TransitiveReduction. + sorted := make([]Vertex, 0, len(g.vertices)) + + // tmp track the current working node to check for cycles + tmp := map[Vertex]bool{} + + // perm tracks completed nodes to end the recursion + perm := map[Vertex]bool{} + + var visit func(v Vertex) + + visit = func(v Vertex) { + if perm[v] { + return + } + + if tmp[v] { + panic("cycle found in dag") + } + + tmp[v] = true + var next Set + switch { + case order&downOrder != 0: + next = g.downEdgesNoCopy(v) + case order&upOrder != 0: + next = g.upEdgesNoCopy(v) + default: + panic(fmt.Sprintln("invalid order", order)) + } + + for _, u := range next { + visit(u) + } + + tmp[v] = false + perm[v] = true + sorted = append(sorted, v) + } + + for _, v := range g.Vertices() { + visit(v) + } + + return sorted +} + type walkType uint64 const ( diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 6e9cf5ac4388..5b273938e8fe 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -429,7 +429,7 @@ func TestAcyclicGraphWalkOrder(t *testing.T) { */ var g AcyclicGraph - for i := 0; i <= 11; i++ { + for i := 1; i <= 11; i++ { g.Add(i) } g.Connect(BasicEdge(1, 3)) @@ -509,6 +509,41 @@ func TestAcyclicGraphWalkOrder(t *testing.T) { t.Errorf("expected visits:\n%v\ngot:\n%v\n", expect, visits) } }) + + t.Run("TopologicalOrder", func(t *testing.T) { + order := g.topoOrder(downOrder) + + // Validate the order by checking it against the initial graph. We only + // need to verify that each node has it's direct dependencies + // satisfied. + completed := map[Vertex]bool{} + for _, v := range order { + deps := g.DownEdges(v) + for _, dep := range deps { + if !completed[dep] { + t.Fatalf("walking node %v, but dependency %v was not yet seen", v, dep) + } + } + completed[v] = true + } + }) + t.Run("ReverseTopologicalOrder", func(t *testing.T) { + order := g.topoOrder(upOrder) + + // Validate the order by checking it against the initial graph. We only + // need to verify that each node has it's direct dependencies + // satisfied. + completed := map[Vertex]bool{} + for _, v := range order { + deps := g.UpEdges(v) + for _, dep := range deps { + if !completed[dep] { + t.Fatalf("walking node %v, but dependency %v was not yet seen", v, dep) + } + } + completed[v] = true + } + }) } const testGraphTransReductionStr = ` From 0e64be0cffea608b4f46761ac18b2bad51540a14 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 22 Jul 2022 12:51:55 -0400 Subject: [PATCH 3/4] use TopologicalOrder for evaluating moves The dag package did not previously provide a topological walk of a given graph. While the existing combination of a transitive reduction with a depth-first walk appeared to accomplish this, depth-first is only equivalent with a simple tree. If there are multiple paths to a node, a depth-first approach will skip dependencies from alternate paths. --- internal/refactoring/move_execute.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index db62152f330f..76d152456f5a 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -92,7 +92,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { } } - g.ReverseDepthFirstWalk(startNodes, func(v dag.Vertex, depth int) error { + for _, v := range g.ReverseTopologicalOrder() { stmt := v.(*MoveStatement) for _, ms := range state.Modules { @@ -187,9 +187,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { panic(fmt.Sprintf("unhandled move object kind %s", kind)) } } - - return nil - }) + } return ret } From 1aa006e80bc0089450243f32de831f8f29ea1c39 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 22 Jul 2022 15:51:31 -0400 Subject: [PATCH 4/4] manual backport of move_execute tests from 31499 --- internal/refactoring/move_execute_test.go | 315 +++++++++------------- 1 file changed, 125 insertions(+), 190 deletions(-) diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index 3f568ee79da8..9d60ff6b60e9 100644 --- a/internal/refactoring/move_execute_test.go +++ b/internal/refactoring/move_execute_test.go @@ -20,120 +20,12 @@ func TestApplyMoves(t *testing.T) { Provider: addrs.MustParseProviderSourceString("example.com/foo/bar"), } - moduleBoo, _ := addrs.ParseModuleInstanceStr("module.boo") - moduleBar, _ := addrs.ParseModuleInstanceStr("module.bar") - moduleBarKey, _ := addrs.ParseModuleInstanceStr("module.bar[0]") - moduleBooHoo, _ := addrs.ParseModuleInstanceStr("module.boo.module.hoo") - moduleBarHoo, _ := addrs.ParseModuleInstanceStr("module.bar.module.hoo") - - instAddrs := map[string]addrs.AbsResourceInstance{ - "foo.from": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), - - "foo.mid": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "mid", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), - - "foo.to": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "to", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), - - "foo.from[0]": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance), - - "foo.to[0]": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "to", - }.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance), - - "module.boo.foo.from": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.NoKey).Absolute(moduleBoo), - - "module.boo.foo.mid": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "mid", - }.Instance(addrs.NoKey).Absolute(moduleBoo), - - "module.boo.foo.to": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "to", - }.Instance(addrs.NoKey).Absolute(moduleBoo), - - "module.boo.foo.from[0]": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.IntKey(0)).Absolute(moduleBoo), - - "module.boo.foo.to[0]": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "to", - }.Instance(addrs.IntKey(0)).Absolute(moduleBoo), - - "module.bar.foo.from": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.NoKey).Absolute(moduleBar), - - "module.bar[0].foo.from": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.NoKey).Absolute(moduleBarKey), - - "module.bar[0].foo.mid": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "mid", - }.Instance(addrs.NoKey).Absolute(moduleBarKey), - - "module.bar[0].foo.to": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "to", - }.Instance(addrs.NoKey).Absolute(moduleBarKey), - - "module.bar[0].foo.from[0]": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.IntKey(0)).Absolute(moduleBarKey), - - "module.bar[0].foo.to[0]": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "to", - }.Instance(addrs.IntKey(0)).Absolute(moduleBarKey), - - "module.boo.module.hoo.foo.from": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.NoKey).Absolute(moduleBooHoo), - - "module.bar.module.hoo.foo.from": addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "from", - }.Instance(addrs.NoKey).Absolute(moduleBarHoo), + mustParseInstAddr := func(s string) addrs.AbsResourceInstance { + addr, err := addrs.ParseAbsResourceInstanceStr(s) + if err != nil { + t.Fatal(err) + } + return addr } emptyResults := MoveResults{ @@ -158,7 +50,7 @@ func TestApplyMoves(t *testing.T) { []MoveStatement{}, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -177,7 +69,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -187,9 +79,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["foo.to"].UniqueKey(): { - From: instAddrs["foo.from"], - To: instAddrs["foo.to"], + mustParseInstAddr("foo.to").UniqueKey(): { + From: mustParseInstAddr("foo.from"), + To: mustParseInstAddr("foo.to"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -204,7 +96,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["foo.from[0]"], + mustParseInstAddr("foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -214,9 +106,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["foo.to[0]"].UniqueKey(): { - From: instAddrs["foo.from[0]"], - To: instAddrs["foo.to[0]"], + mustParseInstAddr("foo.to[0]").UniqueKey(): { + From: mustParseInstAddr("foo.from[0]"), + To: mustParseInstAddr("foo.to[0]"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -232,7 +124,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -242,9 +134,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["foo.to"].UniqueKey(): { - From: instAddrs["foo.from"], - To: instAddrs["foo.to"], + mustParseInstAddr("foo.to").UniqueKey(): { + From: mustParseInstAddr("foo.from"), + To: mustParseInstAddr("foo.to"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -260,7 +152,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["foo.from[0]"], + mustParseInstAddr("foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -270,9 +162,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.boo.foo.to[0]"].UniqueKey(): { - From: instAddrs["foo.from[0]"], - To: instAddrs["module.boo.foo.to[0]"], + mustParseInstAddr("module.boo.foo.to[0]").UniqueKey(): { + From: mustParseInstAddr("foo.from[0]"), + To: mustParseInstAddr("module.boo.foo.to[0]"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -288,7 +180,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.boo.foo.from[0]"], + mustParseInstAddr("module.boo.foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -298,9 +190,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + mustParseInstAddr("module.bar[0].foo.to[0]").UniqueKey(): { + From: mustParseInstAddr("module.boo.foo.from[0]"), + To: mustParseInstAddr("module.bar[0].foo.to[0]"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -316,7 +208,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.boo.foo.from"], + mustParseInstAddr("module.boo.foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -324,7 +216,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["module.boo.module.hoo.foo.from"], + mustParseInstAddr("module.boo.module.hoo.foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -334,13 +226,13 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar.foo.from"].UniqueKey(): { - From: instAddrs["module.boo.foo.from"], - To: instAddrs["module.bar.foo.from"], + mustParseInstAddr("module.bar.foo.from").UniqueKey(): { + From: mustParseInstAddr("module.boo.foo.from"), + To: mustParseInstAddr("module.bar.foo.from"), }, - instAddrs["module.bar.module.hoo.foo.from"].UniqueKey(): { - From: instAddrs["module.boo.module.hoo.foo.from"], - To: instAddrs["module.bar.module.hoo.foo.from"], + mustParseInstAddr("module.bar.module.hoo.foo.from").UniqueKey(): { + From: mustParseInstAddr("module.boo.module.hoo.foo.from"), + To: mustParseInstAddr("module.bar.module.hoo.foo.from"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -357,7 +249,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.boo.foo.from[0]"], + mustParseInstAddr("module.boo.foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -367,9 +259,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.from[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.from[0]"], + mustParseInstAddr("module.bar[0].foo.from[0]").UniqueKey(): { + From: mustParseInstAddr("module.boo.foo.from[0]"), + To: mustParseInstAddr("module.bar[0].foo.from[0]"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -386,7 +278,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.boo.foo.from[0]"], + mustParseInstAddr("module.boo.foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -396,9 +288,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + mustParseInstAddr("module.bar[0].foo.to[0]").UniqueKey(): { + From: mustParseInstAddr("module.boo.foo.from[0]"), + To: mustParseInstAddr("module.bar[0].foo.to[0]"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -415,7 +307,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.boo.foo.from[0]"], + mustParseInstAddr("module.boo.foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -425,9 +317,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + mustParseInstAddr("module.bar[0].foo.to[0]").UniqueKey(): { + From: mustParseInstAddr("module.boo.foo.from[0]"), + To: mustParseInstAddr("module.bar[0].foo.to[0]"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -443,7 +335,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.bar[0].foo.from"], + mustParseInstAddr("module.bar[0].foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -451,7 +343,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["module.boo.foo.to[0]"], + mustParseInstAddr("module.boo.foo.to[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -464,9 +356,9 @@ func TestApplyMoves(t *testing.T) { // occupied by another module. Changes: map[addrs.UniqueKey]MoveSuccess{}, Blocked: map[addrs.UniqueKey]MoveBlocked{ - instAddrs["module.bar[0].foo.from"].Module.UniqueKey(): { - Wanted: instAddrs["module.boo.foo.to[0]"].Module, - Actual: instAddrs["module.bar[0].foo.from"].Module, + mustParseInstAddr("module.bar[0].foo.from").Module.UniqueKey(): { + Wanted: mustParseInstAddr("module.boo.foo.to[0]").Module, + Actual: mustParseInstAddr("module.bar[0].foo.from").Module, }, }, }, @@ -482,7 +374,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -490,7 +382,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["foo.to"], + mustParseInstAddr("foo.to"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -503,9 +395,9 @@ func TestApplyMoves(t *testing.T) { // occupied by another resource. Changes: map[addrs.UniqueKey]MoveSuccess{}, Blocked: map[addrs.UniqueKey]MoveBlocked{ - instAddrs["foo.from"].ContainingResource().UniqueKey(): { - Wanted: instAddrs["foo.to"].ContainingResource(), - Actual: instAddrs["foo.from"].ContainingResource(), + mustParseInstAddr("foo.from").ContainingResource().UniqueKey(): { + Wanted: mustParseInstAddr("foo.to").ContainingResource(), + Actual: mustParseInstAddr("foo.from").ContainingResource(), }, }, }, @@ -521,7 +413,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -529,7 +421,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["foo.to[0]"], + mustParseInstAddr("foo.to[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -542,9 +434,9 @@ func TestApplyMoves(t *testing.T) { // occupied by another resource instance. Changes: map[addrs.UniqueKey]MoveSuccess{}, Blocked: map[addrs.UniqueKey]MoveBlocked{ - instAddrs["foo.from"].UniqueKey(): { - Wanted: instAddrs["foo.to[0]"], - Actual: instAddrs["foo.from"], + mustParseInstAddr("foo.from").UniqueKey(): { + Wanted: mustParseInstAddr("foo.to[0]"), + Actual: mustParseInstAddr("foo.from"), }, }, }, @@ -560,7 +452,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.boo.foo.from"], + mustParseInstAddr("module.boo.foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -570,9 +462,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.to"].UniqueKey(): { - From: instAddrs["module.boo.foo.from"], - To: instAddrs["module.bar[0].foo.to"], + mustParseInstAddr("module.bar[0].foo.to").UniqueKey(): { + From: mustParseInstAddr("module.boo.foo.from"), + To: mustParseInstAddr("module.bar[0].foo.to"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -589,7 +481,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.bar[0].foo.to"], + mustParseInstAddr("module.bar[0].foo.to"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -597,7 +489,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -607,13 +499,13 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.boo.foo.from"].UniqueKey(): { - instAddrs["foo.from"], - instAddrs["module.boo.foo.from"], + mustParseInstAddr("module.boo.foo.from").UniqueKey(): { + mustParseInstAddr("foo.from"), + mustParseInstAddr("module.boo.foo.from"), }, - instAddrs["module.boo.foo.to"].UniqueKey(): { - instAddrs["module.bar[0].foo.to"], - instAddrs["module.boo.foo.to"], + mustParseInstAddr("module.boo.foo.to").UniqueKey(): { + mustParseInstAddr("module.bar[0].foo.to"), + mustParseInstAddr("module.boo.foo.to"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{}, @@ -624,6 +516,49 @@ func TestApplyMoves(t *testing.T) { }, }, + "move resources into module and then move module": { + []MoveStatement{ + testMoveStatement(t, "", "foo.from", "module.boo.foo.to"), + testMoveStatement(t, "", "bar.from", "module.boo.bar.to"), + testMoveStatement(t, "", "module.boo", "module.bar[0]"), + }, + states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + mustParseInstAddr("foo.from"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + s.SetResourceInstanceCurrent( + mustParseInstAddr("bar.from"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + }), + MoveResults{ + Changes: map[addrs.UniqueKey]MoveSuccess{ + mustParseInstAddr("module.bar[0].foo.to").UniqueKey(): MoveSuccess{ + mustParseInstAddr("foo.from"), + mustParseInstAddr("module.bar[0].foo.to"), + }, + mustParseInstAddr("module.bar[0].bar.to").UniqueKey(): MoveSuccess{ + mustParseInstAddr("bar.from"), + mustParseInstAddr("module.bar[0].bar.to"), + }, + }, + Blocked: emptyResults.Blocked, + }, + []string{ + `module.bar[0].bar.to`, + `module.bar[0].foo.to`, + }, + }, + "module move collides with resource move": { []MoveStatement{ testMoveStatement(t, "", "module.bar[0]", "module.boo"), @@ -631,7 +566,7 @@ func TestApplyMoves(t *testing.T) { }, states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( - instAddrs["module.bar[0].foo.from"], + mustParseInstAddr("module.bar[0].foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -639,7 +574,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -650,15 +585,15 @@ func TestApplyMoves(t *testing.T) { MoveResults{ Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.boo.foo.from"].UniqueKey(): { - instAddrs["module.bar[0].foo.from"], - instAddrs["module.boo.foo.from"], + mustParseInstAddr("module.boo.foo.from").UniqueKey(): { + mustParseInstAddr("module.bar[0].foo.from"), + mustParseInstAddr("module.boo.foo.from"), }, }, Blocked: map[addrs.UniqueKey]MoveBlocked{ - instAddrs["foo.from"].ContainingResource().UniqueKey(): { - Actual: instAddrs["foo.from"].ContainingResource(), - Wanted: instAddrs["module.boo.foo.from"].ContainingResource(), + mustParseInstAddr("foo.from").ContainingResource().UniqueKey(): { + Actual: mustParseInstAddr("foo.from").ContainingResource(), + Wanted: mustParseInstAddr("module.boo.foo.from").ContainingResource(), }, }, },