From 95019e3d0225caaf027d889a922bcae68de5cb24 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 | 145 +++++++++++++++++++++++++-------------- internal/dag/dag_test.go | 113 +++++++++++++++++++++++------- 2 files changed, 184 insertions(+), 74 deletions(-) diff --git a/internal/dag/dag.go b/internal/dag/dag.go index 6da10df51b..fd086f6845 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,65 +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 { - seen := make(map[Vertex]struct{}) - frontier := make([]*vertexAtDepth, 0, len(start)) - for _, v := range start { - frontier = append(frontier, &vertexAtDepth{ - Vertex: v, - Depth: 0, - }) - } - 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{}{} - - // Visit the current node - if err := f(current.Vertex, current.Depth); err != nil { - return err - } - - for _, v := range g.downEdgesNoCopy(current.Vertex) { - frontier = append(frontier, &vertexAtDepth{ - Vertex: v, - Depth: current.Depth + 1, - }) - } - } - - return nil + return g.walk(depthFirst|downOrder, false, start, f) } // 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 { + 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 { @@ -244,18 +252,53 @@ func (g *AcyclicGraph) ReverseDepthFirstWalk(start Set, f DepthWalkFunc) error { } seen[current.Vertex] = struct{}{} - for _, t := range g.upEdgesNoCopy(current.Vertex) { - frontier = append(frontier, &vertexAtDepth{ - Vertex: t, - Depth: current.Depth + 1, - }) - } - // Visit the current node if err := f(current.Vertex, current.Depth); err != nil { return err } - } + 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 } + +func appendNext(frontier []vertexAtDepth, next Set, depth int) []vertexAtDepth { + for _, v := range next { + frontier = append(frontier, vertexAtDepth{ + Vertex: v, + Depth: depth, + }) + } + return frontier +} + +func testAppendNextSorted(frontier []vertexAtDepth, edges Set, depth int) []vertexAtDepth { + var newEdges []vertexAtDepth + for _, v := range edges { + newEdges = append(newEdges, vertexAtDepth{ + Vertex: v, + Depth: depth, + }) + } + 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 75cfb86ff0..6e9cf5ac43 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) { +func TestAcyclicGraphWalkOrder(t *testing.T) { + /* Sample dependency graph, + all edges pointing downwards. + 1 2 + / \ / \ + 3 4 5 + / \ / + 6 7 + / | \ + 8 9 10 + \ | / + 11 + */ + var g AcyclicGraph - g.Add(1) - g.Add(2) - g.Add(3) - g.Connect(BasicEdge(3, 2)) - g.Connect(BasicEdge(2, 1)) + 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)) - var visits []Vertex - var lock sync.Mutex - root := make(Set) - root.Add(1) + start := make(Set) + start.Add(2) + start.Add(1) + reverse := make(Set) + reverse.Add(11) + reverse.Add(6) - err := g.ReverseDepthFirstWalk(root, func(v Vertex, d int) error { - lock.Lock() - defer lock.Unlock() - visits = append(visits, v) - g.Remove(v) - return nil + 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) + } }) - if err != nil { - t.Fatalf("err: %s", err) - } + 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 - expected := []Vertex{1, 2, 3} - if !reflect.DeepEqual(visits, expected) { - t.Fatalf("expected: %#v, got: %#v", expected, visits) - } + }) + 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 ca272b21072cb291675d8d6b4d0e350f797e4f14 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 fd086f6845..f5268e76f0 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 6e9cf5ac43..5b273938e8 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 dc9d7108dddae3d83e2b7455380a48d38b7631f4 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 +- internal/refactoring/move_execute_test.go | 321 +++++++++------------- 2 files changed, 130 insertions(+), 197 deletions(-) diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 1880da00a0..9a6d577cfb 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -87,7 +87,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 { @@ -182,9 +182,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { panic(fmt.Sprintf("unhandled move object kind %s", kind)) } } - - return nil - }) + } return ret } diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index 7a8185b18d..f1e4cd04a2 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 := makeMoveResults() @@ -155,7 +47,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(`{}`), @@ -174,7 +66,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(`{}`), @@ -184,9 +76,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["foo.to"], MoveSuccess{ - From: instAddrs["foo.from"], - To: instAddrs["foo.to"], + addrs.MakeMapElem(mustParseInstAddr("foo.to"), MoveSuccess{ + From: mustParseInstAddr("foo.from"), + To: mustParseInstAddr("foo.to"), }), ), Blocked: emptyResults.Blocked, @@ -201,7 +93,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(`{}`), @@ -211,9 +103,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["foo.to[0]"], MoveSuccess{ - From: instAddrs["foo.from[0]"], - To: instAddrs["foo.to[0]"], + addrs.MakeMapElem(mustParseInstAddr("foo.to[0]"), MoveSuccess{ + From: mustParseInstAddr("foo.from[0]"), + To: mustParseInstAddr("foo.to[0]"), }), ), Blocked: emptyResults.Blocked, @@ -229,7 +121,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(`{}`), @@ -239,9 +131,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["foo.to"], MoveSuccess{ - From: instAddrs["foo.from"], - To: instAddrs["foo.to"], + addrs.MakeMapElem(mustParseInstAddr("foo.to"), MoveSuccess{ + From: mustParseInstAddr("foo.from"), + To: mustParseInstAddr("foo.to"), }), ), Blocked: emptyResults.Blocked, @@ -257,7 +149,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(`{}`), @@ -267,9 +159,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.boo.foo.to[0]"], MoveSuccess{ - From: instAddrs["foo.from[0]"], - To: instAddrs["module.boo.foo.to[0]"], + addrs.MakeMapElem(mustParseInstAddr("module.boo.foo.to[0]"), MoveSuccess{ + From: mustParseInstAddr("foo.from[0]"), + To: mustParseInstAddr("module.boo.foo.to[0]"), }), ), Blocked: emptyResults.Blocked, @@ -285,7 +177,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(`{}`), @@ -295,9 +187,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.bar[0].foo.to[0]"], MoveSuccess{ - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to[0]"), MoveSuccess{ + From: mustParseInstAddr("module.boo.foo.from[0]"), + To: mustParseInstAddr("module.bar[0].foo.to[0]"), }), ), Blocked: emptyResults.Blocked, @@ -313,7 +205,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(`{}`), @@ -321,7 +213,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(`{}`), @@ -331,13 +223,13 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.bar.foo.from"], MoveSuccess{ - From: instAddrs["module.boo.foo.from"], - To: instAddrs["module.bar.foo.from"], + addrs.MakeMapElem(mustParseInstAddr("module.bar.foo.from"), MoveSuccess{ + From: mustParseInstAddr("module.boo.foo.from"), + To: mustParseInstAddr("module.bar.foo.from"), }), - addrs.MakeMapElem(instAddrs["module.bar.module.hoo.foo.from"], MoveSuccess{ - From: instAddrs["module.boo.module.hoo.foo.from"], - To: instAddrs["module.bar.module.hoo.foo.from"], + addrs.MakeMapElem(mustParseInstAddr("module.bar.module.hoo.foo.from"), MoveSuccess{ + From: mustParseInstAddr("module.boo.module.hoo.foo.from"), + To: mustParseInstAddr("module.bar.module.hoo.foo.from"), }), ), Blocked: emptyResults.Blocked, @@ -354,7 +246,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(`{}`), @@ -364,9 +256,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.bar[0].foo.from[0]"], MoveSuccess{ - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.from[0]"], + addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.from[0]"), MoveSuccess{ + From: mustParseInstAddr("module.boo.foo.from[0]"), + To: mustParseInstAddr("module.bar[0].foo.from[0]"), }), ), Blocked: emptyResults.Blocked, @@ -383,7 +275,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(`{}`), @@ -393,9 +285,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.bar[0].foo.to[0]"], MoveSuccess{ - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to[0]"), MoveSuccess{ + From: mustParseInstAddr("module.boo.foo.from[0]"), + To: mustParseInstAddr("module.bar[0].foo.to[0]"), }), ), Blocked: emptyResults.Blocked, @@ -412,7 +304,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(`{}`), @@ -422,9 +314,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.bar[0].foo.to[0]"], MoveSuccess{ - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to[0]"), MoveSuccess{ + From: mustParseInstAddr("module.boo.foo.from[0]"), + To: mustParseInstAddr("module.bar[0].foo.to[0]"), }), ), Blocked: emptyResults.Blocked, @@ -440,7 +332,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(`{}`), @@ -448,7 +340,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(`{}`), @@ -462,10 +354,10 @@ func TestApplyMoves(t *testing.T) { Changes: emptyResults.Changes, Blocked: addrs.MakeMap( addrs.MakeMapElem[addrs.AbsMoveable]( - instAddrs["module.bar[0].foo.from"].Module, + mustParseInstAddr("module.bar[0].foo.from").Module, MoveBlocked{ - Wanted: instAddrs["module.boo.foo.to[0]"].Module, - Actual: instAddrs["module.bar[0].foo.from"].Module, + 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(`{}`), @@ -504,10 +396,10 @@ func TestApplyMoves(t *testing.T) { Changes: emptyResults.Changes, Blocked: addrs.MakeMap( addrs.MakeMapElem[addrs.AbsMoveable]( - instAddrs["foo.from"].ContainingResource(), + mustParseInstAddr("foo.from").ContainingResource(), MoveBlocked{ - Wanted: instAddrs["foo.to"].ContainingResource(), - Actual: instAddrs["foo.from"].ContainingResource(), + Wanted: mustParseInstAddr("foo.to").ContainingResource(), + Actual: mustParseInstAddr("foo.from").ContainingResource(), }, ), ), @@ -524,7 +416,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(`{}`), @@ -532,7 +424,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["foo.to[0]"], + mustParseInstAddr("foo.to[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -546,10 +438,10 @@ func TestApplyMoves(t *testing.T) { Changes: emptyResults.Changes, Blocked: addrs.MakeMap( addrs.MakeMapElem[addrs.AbsMoveable]( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), MoveBlocked{ - Wanted: instAddrs["foo.to[0]"], - Actual: instAddrs["foo.from"], + Wanted: mustParseInstAddr("foo.to[0]"), + Actual: mustParseInstAddr("foo.from"), }, ), ), @@ -566,7 +458,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(`{}`), @@ -576,9 +468,9 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.bar[0].foo.to"], MoveSuccess{ - From: instAddrs["module.boo.foo.from"], - To: instAddrs["module.bar[0].foo.to"], + addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to"), MoveSuccess{ + From: mustParseInstAddr("module.boo.foo.from"), + To: mustParseInstAddr("module.bar[0].foo.to"), }), ), Blocked: emptyResults.Blocked, @@ -595,7 +487,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(`{}`), @@ -603,7 +495,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -613,13 +505,13 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.boo.foo.from"], MoveSuccess{ - instAddrs["foo.from"], - instAddrs["module.boo.foo.from"], + addrs.MakeMapElem(mustParseInstAddr("module.boo.foo.from"), MoveSuccess{ + mustParseInstAddr("foo.from"), + mustParseInstAddr("module.boo.foo.from"), }), - addrs.MakeMapElem(instAddrs["module.boo.foo.to"], MoveSuccess{ - instAddrs["module.bar[0].foo.to"], - instAddrs["module.boo.foo.to"], + addrs.MakeMapElem(mustParseInstAddr("module.boo.foo.to"), MoveSuccess{ + mustParseInstAddr("module.bar[0].foo.to"), + mustParseInstAddr("module.boo.foo.to"), }), ), Blocked: emptyResults.Blocked, @@ -630,14 +522,15 @@ func TestApplyMoves(t *testing.T) { }, }, - "module move collides with resource move": { + "move resources into module and then move module": { []MoveStatement{ - testMoveStatement(t, "", "module.bar[0]", "module.boo"), - testMoveStatement(t, "", "foo.from", "module.boo.foo.from"), + 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( - instAddrs["module.bar[0].foo.from"], + mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -645,7 +538,7 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) s.SetResourceInstanceCurrent( - instAddrs["foo.from"], + mustParseInstAddr("bar.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), @@ -655,17 +548,59 @@ func TestApplyMoves(t *testing.T) { }), MoveResults{ Changes: addrs.MakeMap( - addrs.MakeMapElem(instAddrs["module.boo.foo.from"], MoveSuccess{ - instAddrs["module.bar[0].foo.from"], - instAddrs["module.boo.foo.from"], + addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to"), MoveSuccess{ + mustParseInstAddr("foo.from"), + mustParseInstAddr("module.bar[0].foo.to"), + }), + addrs.MakeMapElem(mustParseInstAddr("module.bar[0].bar.to"), 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"), + testMoveStatement(t, "", "foo.from", "module.boo.foo.from"), + }, + states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + mustParseInstAddr("module.bar[0].foo.from"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + s.SetResourceInstanceCurrent( + mustParseInstAddr("foo.from"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + }), + MoveResults{ + Changes: addrs.MakeMap( + addrs.MakeMapElem(mustParseInstAddr("module.boo.foo.from"), MoveSuccess{ + mustParseInstAddr("module.bar[0].foo.from"), + mustParseInstAddr("module.boo.foo.from"), }), ), Blocked: addrs.MakeMap( addrs.MakeMapElem[addrs.AbsMoveable]( - instAddrs["foo.from"].ContainingResource(), + mustParseInstAddr("foo.from").ContainingResource(), MoveBlocked{ - Actual: instAddrs["foo.from"].ContainingResource(), - Wanted: instAddrs["module.boo.foo.from"].ContainingResource(), + Actual: mustParseInstAddr("foo.from").ContainingResource(), + Wanted: mustParseInstAddr("module.boo.foo.from").ContainingResource(), }, ), ), From e3ca4be4b9347b0b50869c5eda949ffc56322a47 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 22 Jul 2022 13:46:06 -0400 Subject: [PATCH 4/4] goimports --- internal/refactoring/move_execute_test.go | 1 + internal/refactoring/move_statement_test.go | 1 + internal/refactoring/move_validate.go | 1 + internal/refactoring/move_validate_test.go | 3 ++- 4 files changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index f1e4cd04a2..edc8afb9fd 100644 --- a/internal/refactoring/move_execute_test.go +++ b/internal/refactoring/move_execute_test.go @@ -10,6 +10,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/states" ) diff --git a/internal/refactoring/move_statement_test.go b/internal/refactoring/move_statement_test.go index 249d7df7eb..a6f0f9f6ee 100644 --- a/internal/refactoring/move_statement_test.go +++ b/internal/refactoring/move_statement_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index 7d00686aec..5fb646ef20 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/dag" diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 2a61bdc6c2..6f57b8e2c6 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -7,6 +7,8 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/zclconf/go-cty/cty/gocty" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configload" @@ -14,7 +16,6 @@ import ( "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/registry" "github.com/hashicorp/terraform/internal/tfdiags" - "github.com/zclconf/go-cty/cty/gocty" ) func TestValidateMoves(t *testing.T) {