diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 66c0d6c0a..322569803 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -10,10 +10,6 @@ import ( "github.com/hashicorp/terraform/internal/states" ) -type MoveResult struct { - From, To addrs.AbsResourceInstance -} - // ApplyMoves modifies in-place the given state object so that any existing // objects that are matched by a "from" argument of one of the move statements // will be moved to instead appear at the "to" argument of that statement. @@ -29,8 +25,11 @@ type MoveResult struct { // // ApplyMoves expects exclusive access to the given state while it's running. // Don't read or write any part of the state structure until ApplyMoves returns. -func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey]MoveResult { - results := make(map[addrs.UniqueKey]MoveResult) +func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { + ret := MoveResults{ + Changes: make(map[addrs.UniqueKey]MoveSuccess), + Blocked: make(map[addrs.UniqueKey]MoveBlocked), + } // The methodology here is to construct a small graph of all of the move // statements where the edges represent where a particular statement @@ -44,7 +43,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] // at all. The separate validation step should detect this and return // an error. if len(g.Cycles()) != 0 { - return results + return ret } // The starting nodes are the ones that don't depend on any other nodes. @@ -57,11 +56,33 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] if startNodes.Len() == 0 { log.Println("[TRACE] refactoring.ApplyMoves: No 'moved' statements to consider in this configuration") - return results + return ret } log.Printf("[TRACE] refactoring.ApplyMoves: Processing 'moved' statements in the configuration\n%s", logging.Indent(g.String())) + recordOldAddr := func(oldAddr, newAddr addrs.AbsResourceInstance) { + oldAddrKey := oldAddr.UniqueKey() + newAddrKey := newAddr.UniqueKey() + if prevMove, exists := ret.Changes[oldAddrKey]; exists { + // If the old address was _already_ the result of a move then + // we'll replace that entry so that our results summarize a chain + // of moves into a single entry. + delete(ret.Changes, oldAddrKey) + oldAddr = prevMove.From + } + ret.Changes[newAddrKey] = MoveSuccess{ + From: oldAddr, + To: newAddr, + } + } + recordBlockage := func(newAddr, wantedAddr addrs.AbsMoveable) { + ret.Blocked[newAddr.UniqueKey()] = MoveBlocked{ + Wanted: wantedAddr, + Actual: newAddr, + } + } + g.ReverseDepthFirstWalk(startNodes, func(v dag.Vertex, depth int) error { stmt := v.(*MoveStatement) @@ -83,11 +104,9 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] // If we already have a module at the new address then // we'll skip this move and let the existing object take // priority. - // TODO: This should probably generate a user-visible - // warning, but we'd need to rethink the signature of this - // function to achieve that. if ms := state.Module(newAddr); ms != nil { log.Printf("[WARN] Skipped moving %s to %s, because there's already another module instance at the destination", modAddr, newAddr) + recordBlockage(modAddr, newAddr) continue } @@ -98,12 +117,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] for key := range rs.Instances { oldInst := relAddr.Instance(key).Absolute(modAddr) newInst := relAddr.Instance(key).Absolute(newAddr) - result := MoveResult{ - From: oldInst, - To: newInst, - } - results[oldInst.UniqueKey()] = result - results[newInst.UniqueKey()] = result + recordOldAddr(oldInst, newInst) } } @@ -121,23 +135,16 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] // If we already have a resource at the new address then // we'll skip this move and let the existing object take // priority. - // TODO: This should probably generate a user-visible - // warning, but we'd need to rethink the signature of this - // function to achieve that. if rs := state.Resource(newAddr); rs != nil { log.Printf("[WARN] Skipped moving %s to %s, because there's already another resource at the destination", rAddr, newAddr) + recordBlockage(rAddr, newAddr) continue } for key := range rs.Instances { oldInst := rAddr.Instance(key) newInst := newAddr.Instance(key) - result := MoveResult{ - From: oldInst, - To: newInst, - } - results[oldInst.UniqueKey()] = result - results[newInst.UniqueKey()] = result + recordOldAddr(oldInst, newInst) } state.MoveAbsResource(rAddr, newAddr) continue @@ -150,17 +157,13 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] // If we already have a resource instance at the new // address then we'll skip this move and let the existing // object take priority. - // TODO: This should probably generate a user-visible - // warning, but we'd need to rethink the signature of this - // function to achieve that. if is := state.ResourceInstance(newAddr); is != nil { log.Printf("[WARN] Skipped moving %s to %s, because there's already another resource instance at the destination", iAddr, newAddr) + recordBlockage(iAddr, newAddr) continue } - result := MoveResult{From: iAddr, To: newAddr} - results[iAddr.UniqueKey()] = result - results[newAddr.UniqueKey()] = result + recordOldAddr(iAddr, newAddr) state.MoveAbsResourceInstance(iAddr, newAddr) continue @@ -175,17 +178,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] return nil }) - // FIXME: In the case of either chained or nested moves, "results" will - // be left in a pretty interesting shape where the "old" address will - // refer to a result that describes only the first step, while the "new" - // address will refer to a result that describes only the last step. - // To make that actually useful we'll need a different strategy where - // the result describes the _effective_ source and destination, skipping - // over any intermediate steps we took to get there, so that ultimately - // we'll have enough information to annotate items in the plan with the - // addresses the originally moved from. - - return results + return ret } // buildMoveStatementGraph constructs a dependency graph of the given move @@ -218,3 +211,67 @@ func buildMoveStatementGraph(stmts []MoveStatement) *dag.AcyclicGraph { return g } + +// MoveResults describes the outcome of an ApplyMoves call. +type MoveResults struct { + // Changes is a map from the unique keys of the final new resource + // instance addresses to an object describing what changed. + // + // This includes one entry for each resource instance address that was + // the destination of a move statement. It doesn't include resource + // instances that were not affected by moves at all, but it does include + // resource instance addresses that were "blocked" (also recorded in + // BlockedAddrs) if and only if they were able to move at least + // partially along a chain before being blocked. + // + // In the return value from ApplyMoves, all of the keys are guaranteed to + // be unique keys derived from addrs.AbsResourceInstance values. + Changes map[addrs.UniqueKey]MoveSuccess + + // Blocked is a map from the unique keys of the final new + // resource instances addresses to information about where they "wanted" + // to move, but were blocked by a pre-existing object at the same address. + // + // "Blocking" can arise in unusual situations where multiple points along + // a move chain were already bound to objects, and thus only one of them + // can actually adopt the final position in the chain. It can also + // occur in other similar situations, such as if a configuration contains + // a move of an entire module and a move of an individual resource into + // that module, such that the individual resource would collide with a + // resource in the whole module that was moved. + // + // In the return value from ApplyMoves, all of the keys are guaranteed to + // be unique keys derived from values of addrs.AbsMoveable types. + Blocked map[addrs.UniqueKey]MoveBlocked +} + +type MoveSuccess struct { + From addrs.AbsResourceInstance + To addrs.AbsResourceInstance +} + +type MoveBlocked struct { + Wanted addrs.AbsMoveable + Actual addrs.AbsMoveable +} + +// AddrMoved returns true if and only if the given resource instance moved to +// a new address in the ApplyMoves call that the receiver is describing. +// +// If AddrMoved returns true, you can pass the same address to method OldAddr +// to find its original address prior to moving. +func (rs MoveResults) AddrMoved(newAddr addrs.AbsResourceInstance) bool { + _, ok := rs.Changes[newAddr.UniqueKey()] + return ok +} + +// OldAddr returns the old address of the given resource instance address, or +// just returns back the same address if the given instance wasn't affected by +// any move statements. +func (rs MoveResults) OldAddr(newAddr addrs.AbsResourceInstance) addrs.AbsResourceInstance { + change, ok := rs.Changes[newAddr.UniqueKey()] + if !ok { + return newAddr + } + return change.From +} diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index f3ab1ec1e..e913df9f4 100644 --- a/internal/refactoring/move_execute_test.go +++ b/internal/refactoring/move_execute_test.go @@ -115,13 +115,16 @@ func TestApplyMoves(t *testing.T) { }.Instance(addrs.IntKey(0)).Absolute(moduleBarKey), } - emptyResults := map[addrs.UniqueKey]MoveResult{} + emptyResults := MoveResults{ + Changes: map[addrs.UniqueKey]MoveSuccess{}, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, + } tests := map[string]struct { Stmts []MoveStatement State *states.State - WantResults map[addrs.UniqueKey]MoveResult + WantResults MoveResults WantInstanceAddrs []string }{ "no moves and empty state": { @@ -161,15 +164,14 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - instAddrs["foo.from"].UniqueKey(): { - From: instAddrs["foo.from"], - To: instAddrs["foo.to"], - }, - instAddrs["foo.to"].UniqueKey(): { - From: instAddrs["foo.from"], - To: instAddrs["foo.to"], + MoveResults{ + Changes: map[addrs.UniqueKey]MoveSuccess{ + instAddrs["foo.to"].UniqueKey(): { + From: instAddrs["foo.from"], + To: instAddrs["foo.to"], + }, }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, []string{ `foo.to`, @@ -189,15 +191,14 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - instAddrs["foo.from[0]"].UniqueKey(): { - From: instAddrs["foo.from[0]"], - To: instAddrs["foo.to[0]"], - }, - instAddrs["foo.to[0]"].UniqueKey(): { - From: instAddrs["foo.from[0]"], - To: instAddrs["foo.to[0]"], + MoveResults{ + Changes: map[addrs.UniqueKey]MoveSuccess{ + instAddrs["foo.to[0]"].UniqueKey(): { + From: instAddrs["foo.from[0]"], + To: instAddrs["foo.to[0]"], + }, }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, []string{ `foo.to[0]`, @@ -218,19 +219,14 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - instAddrs["foo.from"].UniqueKey(): { - From: instAddrs["foo.from"], - To: instAddrs["foo.mid"], - }, - instAddrs["foo.mid"].UniqueKey(): { - From: instAddrs["foo.mid"], - To: instAddrs["foo.to"], - }, - instAddrs["foo.to"].UniqueKey(): { - From: instAddrs["foo.mid"], - To: instAddrs["foo.to"], + MoveResults{ + Changes: map[addrs.UniqueKey]MoveSuccess{ + instAddrs["foo.to"].UniqueKey(): { + From: instAddrs["foo.from"], + To: instAddrs["foo.to"], + }, }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, []string{ `foo.to`, @@ -251,15 +247,14 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - instAddrs["foo.from[0]"].UniqueKey(): { - From: instAddrs["foo.from[0]"], - To: instAddrs["module.boo.foo.to[0]"], - }, - instAddrs["module.boo.foo.to[0]"].UniqueKey(): { - From: instAddrs["foo.from[0]"], - To: instAddrs["module.boo.foo.to[0]"], + 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]"], + }, }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, []string{ `module.boo.foo.to[0]`, @@ -280,15 +275,14 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - instAddrs["module.boo.foo.from[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], - }, - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + 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]"], + }, }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, []string{ `module.bar[0].foo.to[0]`, @@ -309,15 +303,14 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - instAddrs["module.boo.foo.from[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.from[0]"], - }, - instAddrs["module.bar[0].foo.from[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.from[0]"], + 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]"], + }, }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, []string{ `module.bar[0].foo.from[0]`, @@ -339,19 +332,14 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - instAddrs["module.boo.foo.from[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.from[0]"], - }, - instAddrs["module.bar[0].foo.from[0]"].UniqueKey(): { - From: instAddrs["module.bar[0].foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], - }, - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { - From: instAddrs["module.bar[0].foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + 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]"], + }, }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, []string{ `module.bar[0].foo.to[0]`, @@ -373,19 +361,14 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - instAddrs["module.boo.foo.from[0]"].UniqueKey(): { - From: instAddrs["module.boo.foo.from[0]"], - To: instAddrs["module.bar[0].foo.from[0]"], - }, - instAddrs["module.bar[0].foo.from[0]"].UniqueKey(): { - From: instAddrs["module.bar[0].foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], - }, - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { - From: instAddrs["module.bar[0].foo.from[0]"], - To: instAddrs["module.bar[0].foo.to[0]"], + 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]"], + }, }, + Blocked: map[addrs.UniqueKey]MoveBlocked{}, }, []string{ `module.bar[0].foo.to[0]`, @@ -414,9 +397,16 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ + MoveResults{ // Nothing moved, because the module.b address is already // 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, + }, + }, }, []string{ `module.bar[0].foo.from`, @@ -446,9 +436,16 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - // Nothing moved, because the module.b address is already - // occupied by another module. + MoveResults{ + // Nothing moved, because the from.to address is already + // 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(), + }, + }, }, []string{ `foo.from`, @@ -478,22 +475,141 @@ func TestApplyMoves(t *testing.T) { providerAddr, ) }), - map[addrs.UniqueKey]MoveResult{ - // Nothing moved, because the module.b address is already - // occupied by another module. + MoveResults{ + // Nothing moved, because the from.to[0] address is already + // 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"], + }, + }, }, []string{ `foo.from`, `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"), + }, + 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{ + // 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, + }, + }, + []string{ + //`foo.from`, + //`module.boo.foo.from`, + `module.bar[0].foo.to`, + `module.boo.foo.from`, + }, + }, + */ + + // 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( + 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, + }, + }, + []string{ + //`foo.from`, + //`module.boo.foo.from`, + `module.bar[0].foo.from`, + `module.boo.foo.from`, + }, + }, + */ } for name, test := range tests { t.Run(name, func(t *testing.T) { var stmtsBuf strings.Builder for _, stmt := range test.Stmts { - fmt.Fprintf(&stmtsBuf, "- from: %s\n to: %s\n", stmt.From, stmt.To) + fmt.Fprintf(&stmtsBuf, "• from: %s\n to: %s\n", stmt.From, stmt.To) } t.Logf("move statements:\n%s", stmtsBuf.String()) diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 7ec8ab2f9..49f8acd86 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -325,29 +325,6 @@ Each resource can have moved from only one source resource.`, Each resource can have moved from only one source resource.`, }, - /* - // FIXME: This rule requires a deeper analysis to understand that - // module.single already contains a test.single and thus moving - // it to module.foo implicitly also moves module.single.test.single - // module.foo.test.single. - "two different moves to nested test.single by different paths": { - Statements: []MoveStatement{ - makeTestMoveStmt(t, - ``, - `test.beep`, - `module.foo.test.single`, - ), - makeTestMoveStmt(t, - ``, - `module.single`, - `module.foo`, - ), - }, - WantError: `Ambiguous move statements: A statement at test:1,1 declared that test.beep moved to module.foo.test.single, but this statement instead declares that module.single.test.single moved there. - - Each resource can have moved from only one source resource.`, - }, - */ "move from resource in another module package": { Statements: []MoveStatement{ makeTestMoveStmt(t, diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index f70a8f67b..a48676ffa 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -296,7 +296,7 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State return destroyPlan, diags } -func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState *states.State, targets []addrs.Targetable) ([]refactoring.MoveStatement, map[addrs.UniqueKey]refactoring.MoveResult) { +func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState *states.State, targets []addrs.Targetable) ([]refactoring.MoveStatement, refactoring.MoveResults) { explicitMoveStmts := refactoring.FindMoveStatements(config) implicitMoveStmts := refactoring.ImpliedMoveStatements(config, prevRunState, explicitMoveStmts) var moveStmts []refactoring.MoveStatement @@ -309,7 +309,7 @@ func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState return moveStmts, moveResults } -func (c *Context) prePlanVerifyTargetedMoves(moveResults map[addrs.UniqueKey]refactoring.MoveResult, targets []addrs.Targetable) tfdiags.Diagnostics { +func (c *Context) prePlanVerifyTargetedMoves(moveResults refactoring.MoveResults, targets []addrs.Targetable) tfdiags.Diagnostics { if len(targets) < 1 { return nil // the following only matters when targeting } @@ -317,7 +317,7 @@ func (c *Context) prePlanVerifyTargetedMoves(moveResults map[addrs.UniqueKey]ref var diags tfdiags.Diagnostics var excluded []addrs.AbsResourceInstance - for _, result := range moveResults { + for _, result := range moveResults.Changes { fromMatchesTarget := false toMatchesTarget := false for _, targetAddr := range targets { @@ -475,10 +475,10 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, } } -func (c *Context) driftedResources(config *configs.Config, oldState, newState *states.State, moves map[addrs.UniqueKey]refactoring.MoveResult) ([]*plans.ResourceInstanceChangeSrc, tfdiags.Diagnostics) { +func (c *Context) driftedResources(config *configs.Config, oldState, newState *states.State, moves refactoring.MoveResults) ([]*plans.ResourceInstanceChangeSrc, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - if newState.ManagedResourcesEqual(oldState) && len(moves) == 0 { + if newState.ManagedResourcesEqual(oldState) && len(moves.Changes) == 0 { // Nothing to do, because we only detect and report drift for managed // resource instances. return nil, diags @@ -510,7 +510,7 @@ func (c *Context) driftedResources(config *configs.Config, oldState, newState *s // Previous run address defaults to the current address, but // can differ if the resource moved before refreshing prevRunAddr := addr - if move, ok := moves[addr.UniqueKey()]; ok { + if move, ok := moves.Changes[addr.UniqueKey()]; ok { prevRunAddr = move.From } diff --git a/internal/terraform/context_walk.go b/internal/terraform/context_walk.go index e041f80b2..166341513 100644 --- a/internal/terraform/context_walk.go +++ b/internal/terraform/context_walk.go @@ -3,7 +3,6 @@ package terraform import ( "log" - "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/plans" @@ -25,7 +24,7 @@ type graphWalkOpts struct { Config *configs.Config RootVariableValues InputValues - MoveResults map[addrs.UniqueKey]refactoring.MoveResult + MoveResults refactoring.MoveResults } func (c *Context) walk(graph *Graph, operation walkOperation, opts *graphWalkOpts) (*ContextGraphWalker, tfdiags.Diagnostics) { diff --git a/internal/terraform/eval_context.go b/internal/terraform/eval_context.go index 61b4f2448..4b5a3a5c2 100644 --- a/internal/terraform/eval_context.go +++ b/internal/terraform/eval_context.go @@ -174,7 +174,7 @@ type EvalContext interface { // This data structure is created prior to the graph walk and read-only // thereafter, so callers must not modify the returned map or any other // objects accessible through it. - MoveResults() map[addrs.UniqueKey]refactoring.MoveResult + MoveResults() refactoring.MoveResults // WithPath returns a copy of the context with the internal path set to the // path argument. diff --git a/internal/terraform/eval_context_builtin.go b/internal/terraform/eval_context_builtin.go index ea83e8279..ecbac446e 100644 --- a/internal/terraform/eval_context_builtin.go +++ b/internal/terraform/eval_context_builtin.go @@ -69,7 +69,7 @@ type BuiltinEvalContext struct { RefreshStateValue *states.SyncState PrevRunStateValue *states.SyncState InstanceExpanderValue *instances.Expander - MoveResultsValue map[addrs.UniqueKey]refactoring.MoveResult + MoveResultsValue refactoring.MoveResults } // BuiltinEvalContext implements EvalContext @@ -368,6 +368,6 @@ func (ctx *BuiltinEvalContext) InstanceExpander() *instances.Expander { return ctx.InstanceExpanderValue } -func (ctx *BuiltinEvalContext) MoveResults() map[addrs.UniqueKey]refactoring.MoveResult { +func (ctx *BuiltinEvalContext) MoveResults() refactoring.MoveResults { return ctx.MoveResultsValue } diff --git a/internal/terraform/eval_context_mock.go b/internal/terraform/eval_context_mock.go index 52a06c3ee..edcdaac6b 100644 --- a/internal/terraform/eval_context_mock.go +++ b/internal/terraform/eval_context_mock.go @@ -132,7 +132,7 @@ type MockEvalContext struct { PrevRunStateState *states.SyncState MoveResultsCalled bool - MoveResultsResults map[addrs.UniqueKey]refactoring.MoveResult + MoveResultsResults refactoring.MoveResults InstanceExpanderCalled bool InstanceExpanderExpander *instances.Expander @@ -353,7 +353,7 @@ func (c *MockEvalContext) PrevRunState() *states.SyncState { return c.PrevRunStateState } -func (c *MockEvalContext) MoveResults() map[addrs.UniqueKey]refactoring.MoveResult { +func (c *MockEvalContext) MoveResults() refactoring.MoveResults { c.MoveResultsCalled = true return c.MoveResultsResults } diff --git a/internal/terraform/graph_walk_context.go b/internal/terraform/graph_walk_context.go index 39a97032c..9e9e2a88f 100644 --- a/internal/terraform/graph_walk_context.go +++ b/internal/terraform/graph_walk_context.go @@ -25,12 +25,12 @@ type ContextGraphWalker struct { // Configurable values Context *Context - State *states.SyncState // Used for safe concurrent access to state - RefreshState *states.SyncState // Used for safe concurrent access to state - PrevRunState *states.SyncState // Used for safe concurrent access to state - Changes *plans.ChangesSync // Used for safe concurrent writes to changes - InstanceExpander *instances.Expander // Tracks our gradual expansion of module and resource instances - MoveResults map[addrs.UniqueKey]refactoring.MoveResult // Read-only record of earlier processing of move statements + State *states.SyncState // Used for safe concurrent access to state + RefreshState *states.SyncState // Used for safe concurrent access to state + PrevRunState *states.SyncState // Used for safe concurrent access to state + Changes *plans.ChangesSync // Used for safe concurrent writes to changes + InstanceExpander *instances.Expander // Tracks our gradual expansion of module and resource instances + MoveResults refactoring.MoveResults // Read-only record of earlier processing of move statements Operation walkOperation StopContext context.Context RootVariableValues InputValues diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index e73939a8e..3cab2b181 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -2282,13 +2282,5 @@ func (n *NodeAbstractResourceInstance) prevRunAddr(ctx EvalContext) addrs.AbsRes func resourceInstancePrevRunAddr(ctx EvalContext, currentAddr addrs.AbsResourceInstance) addrs.AbsResourceInstance { table := ctx.MoveResults() - - result, ok := table[currentAddr.UniqueKey()] - if !ok { - // If there's no entry in the table then we'll assume it didn't move - // at all, and so its previous address is the same as the current one. - return currentAddr - } - - return result.From + return table.OldAddr(currentAddr) }