diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 322569803..7f5fbae23 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -200,10 +200,13 @@ func buildMoveStatementGraph(stmts []MoveStatement) *dag.AcyclicGraph { for dependerI := range stmts { depender := &stmts[dependerI] for dependeeI := range stmts { + if dependerI == dependeeI { + // skip comparing the statement to itself + continue + } dependee := &stmts[dependeeI] - dependeeTo := dependee.To - dependerFrom := depender.From - if dependerFrom.CanChainFrom(dependeeTo) || dependerFrom.NestedWithin(dependeeTo) { + + if statementDependsOn(depender, dependee) { g.Connect(dag.BasicEdge(depender, dependee)) } } @@ -212,6 +215,36 @@ func buildMoveStatementGraph(stmts []MoveStatement) *dag.AcyclicGraph { return g } +// statementDependsOn returns true if statement a depends on statement b; +// i.e. statement b must be executed before statement a. +func statementDependsOn(a, b *MoveStatement) bool { + // chain-able moves are simple, as on the destination of one move could be + // equal to the source of another. + if a.From.CanChainFrom(b.To) { + return true + } + + // Statement nesting in more complex, as we have 8 possible combinations to + // assess. Here we list all combinations, along with the statement which + // must be executed first when one address is nested within another. + // A.From IsNestedWithin B.From => A + // A.From IsNestedWithin B.To => B + // A.To IsNestedWithin B.From => A + // A.To IsNestedWithin B.To => B + // B.From IsNestedWithin A.From => B + // B.From IsNestedWithin A.To => A + // B.To IsNestedWithin A.From => B + // B.To IsNestedWithin A.To => A + // + // Since we are only interested in checking if A depends on B, we only need + // to check the 4 possibilities above which result in B being executed + // first. + return a.From.NestedWithin(b.To) || + a.To.NestedWithin(b.To) || + b.From.NestedWithin(a.From) || + b.To.NestedWithin(a.From) +} + // MoveResults describes the outcome of an ApplyMoves call. type MoveResults struct { // Changes is a map from the unique keys of the final new resource diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index e913df9f4..ab71f5b0f 100644 --- a/internal/refactoring/move_execute_test.go +++ b/internal/refactoring/move_execute_test.go @@ -491,118 +491,120 @@ func TestApplyMoves(t *testing.T) { `foo.to[0]`, }, }, - - // FIXME: This test seems to flap between the result the test case - // currently records and the "more intuitive" results included inline, - // which suggests we have a missing edge in our move dependency graph. - // (The MoveResults commented out below predates some changes to that - // struct, so will need updating once we uncomment this test.) - /* - "move module and then move resource into it": { - []MoveStatement{ - testMoveStatement(t, "", "module.bar[0]", "module.boo"), - testMoveStatement(t, "", "foo.from", "module.boo.foo.from"), + "move resource and containing module": { + []MoveStatement{ + testMoveStatement(t, "", "module.boo", "module.bar[0]"), + testMoveStatement(t, "boo", "foo.from", "foo.to"), + }, + states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + instAddrs["module.boo.foo.from"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + }), + 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"], + }, }, - states.BuildState(func(s *states.SyncState) { - s.SetResourceInstanceCurrent( + Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }, + []string{ + `module.bar[0].foo.to`, + }, + }, + + "move module and then move resource into it": { + []MoveStatement{ + testMoveStatement(t, "", "module.bar[0]", "module.boo"), + testMoveStatement(t, "", "foo.from", "module.boo.foo.from"), + }, + states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + instAddrs["module.bar[0].foo.to"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + s.SetResourceInstanceCurrent( + instAddrs["foo.from"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + }), + MoveResults{ + Changes: map[addrs.UniqueKey]MoveSuccess{ + instAddrs["module.boo.foo.from"].UniqueKey(): { + instAddrs["foo.from"], + instAddrs["module.boo.foo.from"], + }, + instAddrs["module.boo.foo.to"].UniqueKey(): { instAddrs["module.bar[0].foo.to"], - &states.ResourceInstanceObjectSrc{ - Status: states.ObjectReady, - AttrsJSON: []byte(`{}`), - }, - providerAddr, - ) - s.SetResourceInstanceCurrent( - instAddrs["foo.from"], - &states.ResourceInstanceObjectSrc{ - Status: states.ObjectReady, - AttrsJSON: []byte(`{}`), - }, - providerAddr, - ) - }), - MoveResults{ - // FIXME: This result is counter-intuitive, because ApplyMoves - // handled the resource move first and then the module move - // collided with it. It would be arguably more intuitive to - // complete the module move first and then let the "smaller" - // resource move merge into it. - // (The arguably-more-intuitive results are commented out - // in the maps below.) - - Changes: map[addrs.UniqueKey]MoveSuccess{ - //instAddrs["module.boo.foo.to"].UniqueKey(): instAddrs["module.bar[0].foo.to"], - //instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"], - instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"], - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{ - // intuitive result: nothing blocked - instAddrs["module.bar[0].foo.to"].Module.UniqueKey(): instAddrs["module.boo.foo.from"].Module, + instAddrs["module.boo.foo.to"], }, }, - []string{ - //`foo.from`, - //`module.boo.foo.from`, - `module.bar[0].foo.to`, - `module.boo.foo.from`, - }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, - */ + []string{ + `module.boo.foo.from`, + `module.boo.foo.to`, + }, + }, - // FIXME: This test seems to flap between the result the test case - // currently records and the "more intuitive" results included inline, - // which suggests we have a missing edge in our move dependency graph. - // (The MoveResults commented out below predates some changes to that - // struct, so will need updating once we uncomment this test.) - /* - "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( + "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( + instAddrs["module.bar[0].foo.from"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + s.SetResourceInstanceCurrent( + instAddrs["foo.from"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + }), + MoveResults{ + Changes: map[addrs.UniqueKey]MoveSuccess{ + + instAddrs["module.boo.foo.from"].UniqueKey(): { instAddrs["module.bar[0].foo.from"], - &states.ResourceInstanceObjectSrc{ - Status: states.ObjectReady, - AttrsJSON: []byte(`{}`), - }, - providerAddr, - ) - s.SetResourceInstanceCurrent( - instAddrs["foo.from"], - &states.ResourceInstanceObjectSrc{ - Status: states.ObjectReady, - AttrsJSON: []byte(`{}`), - }, - providerAddr, - ) - }), - MoveResults{ - // FIXME: This result is counter-intuitive, because ApplyMoves - // handled the resource move first and then it was the - // module move that collided, whereas it would arguably have - // been better to let the module take priority and have only - // the one resource move be ignored due to the collision. - // (The arguably-more-intuitive results are commented out - // in the maps below.) - Changes: map[addrs.UniqueKey]MoveSuccess{ - //instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["module.bar[0].foo.from"], - instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"], - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{ - //instAddrs["foo.from"].UniqueKey(): instAddrs["module.bar[0].foo.from"], - instAddrs["module.bar[0].foo.from"].Module.UniqueKey(): instAddrs["module.boo.foo.from"].Module, + instAddrs["module.boo.foo.from"], }, }, - []string{ - //`foo.from`, - //`module.boo.foo.from`, - `module.bar[0].foo.from`, - `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(), + }, }, }, - */ + []string{ + `foo.from`, + `module.boo.foo.from`, + }, + }, } for name, test := range tests {