diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index db62152f330f..1880da00a010 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -26,10 +26,7 @@ import ( // 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) MoveResults { - ret := MoveResults{ - Changes: make(map[addrs.UniqueKey]MoveSuccess), - Blocked: make(map[addrs.UniqueKey]MoveBlocked), - } + ret := makeMoveResults() if len(stmts) == 0 { return ret @@ -71,25 +68,23 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { 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 prevMove, exists := ret.Changes.GetOk(oldAddr); 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) + ret.Changes.Remove(oldAddr) oldAddr = prevMove.From } - ret.Changes[newAddrKey] = MoveSuccess{ + ret.Changes.Put(newAddr, MoveSuccess{ From: oldAddr, To: newAddr, - } + }) } recordBlockage := func(newAddr, wantedAddr addrs.AbsMoveable) { - ret.Blocked[newAddr.UniqueKey()] = MoveBlocked{ + ret.Blocked.Put(newAddr, MoveBlocked{ Wanted: wantedAddr, Actual: newAddr, - } + }) } g.ReverseDepthFirstWalk(startNodes, func(v dag.Vertex, depth int) error { @@ -292,7 +287,7 @@ type MoveResults struct { // // 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 + Changes addrs.Map[addrs.AbsResourceInstance, MoveSuccess] // Blocked is a map from the unique keys of the final new // resource instances addresses to information about where they "wanted" @@ -308,7 +303,14 @@ type MoveResults struct { // // 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 + Blocked addrs.Map[addrs.AbsMoveable, MoveBlocked] +} + +func makeMoveResults() MoveResults { + return MoveResults{ + Changes: addrs.MakeMap[addrs.AbsResourceInstance, MoveSuccess](), + Blocked: addrs.MakeMap[addrs.AbsMoveable, MoveBlocked](), + } } type MoveSuccess struct { @@ -327,15 +329,14 @@ type MoveBlocked struct { // 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 + return rs.Changes.Has(newAddr) } // 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()] + change, ok := rs.Changes.GetOk(newAddr) if !ok { return newAddr } diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index 3f568ee79da8..7a8185b18d60 100644 --- a/internal/refactoring/move_execute_test.go +++ b/internal/refactoring/move_execute_test.go @@ -136,10 +136,7 @@ func TestApplyMoves(t *testing.T) { }.Instance(addrs.NoKey).Absolute(moduleBarHoo), } - emptyResults := MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{}, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, - } + emptyResults := makeMoveResults() tests := map[string]struct { Stmts []MoveStatement @@ -186,13 +183,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["foo.to"].UniqueKey(): { + Changes: addrs.MakeMap( + addrs.MakeMapElem(instAddrs["foo.to"], MoveSuccess{ From: instAddrs["foo.from"], To: instAddrs["foo.to"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `foo.to`, @@ -213,13 +210,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["foo.to[0]"].UniqueKey(): { + Changes: addrs.MakeMap( + addrs.MakeMapElem(instAddrs["foo.to[0]"], MoveSuccess{ From: instAddrs["foo.from[0]"], To: instAddrs["foo.to[0]"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `foo.to[0]`, @@ -241,13 +238,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["foo.to"].UniqueKey(): { + Changes: addrs.MakeMap( + addrs.MakeMapElem(instAddrs["foo.to"], MoveSuccess{ From: instAddrs["foo.from"], To: instAddrs["foo.to"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `foo.to`, @@ -269,13 +266,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.boo.foo.to[0]"].UniqueKey(): { + Changes: addrs.MakeMap( + addrs.MakeMapElem(instAddrs["module.boo.foo.to[0]"], MoveSuccess{ From: instAddrs["foo.from[0]"], To: instAddrs["module.boo.foo.to[0]"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `module.boo.foo.to[0]`, @@ -297,13 +294,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { + 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]"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `module.bar[0].foo.to[0]`, @@ -333,17 +330,17 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar.foo.from"].UniqueKey(): { + Changes: addrs.MakeMap( + addrs.MakeMapElem(instAddrs["module.bar.foo.from"], MoveSuccess{ From: instAddrs["module.boo.foo.from"], To: instAddrs["module.bar.foo.from"], - }, - instAddrs["module.bar.module.hoo.foo.from"].UniqueKey(): { + }), + 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"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `module.bar.foo.from`, @@ -366,13 +363,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.from[0]"].UniqueKey(): { + 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]"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `module.bar[0].foo.from[0]`, @@ -395,13 +392,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { + 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]"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `module.bar[0].foo.to[0]`, @@ -424,13 +421,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): { + 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]"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `module.bar[0].foo.to[0]`, @@ -462,13 +459,16 @@ func TestApplyMoves(t *testing.T) { 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, - }, - }, + Changes: emptyResults.Changes, + Blocked: addrs.MakeMap( + addrs.MakeMapElem[addrs.AbsMoveable]( + instAddrs["module.bar[0].foo.from"].Module, + MoveBlocked{ + Wanted: instAddrs["module.boo.foo.to[0]"].Module, + Actual: instAddrs["module.bar[0].foo.from"].Module, + }, + ), + ), }, []string{ `module.bar[0].foo.from`, @@ -501,13 +501,16 @@ func TestApplyMoves(t *testing.T) { 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(), - }, - }, + Changes: emptyResults.Changes, + Blocked: addrs.MakeMap( + addrs.MakeMapElem[addrs.AbsMoveable]( + instAddrs["foo.from"].ContainingResource(), + MoveBlocked{ + Wanted: instAddrs["foo.to"].ContainingResource(), + Actual: instAddrs["foo.from"].ContainingResource(), + }, + ), + ), }, []string{ `foo.from`, @@ -540,13 +543,16 @@ func TestApplyMoves(t *testing.T) { 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"], - }, - }, + Changes: emptyResults.Changes, + Blocked: addrs.MakeMap( + addrs.MakeMapElem[addrs.AbsMoveable]( + instAddrs["foo.from"], + MoveBlocked{ + Wanted: instAddrs["foo.to[0]"], + Actual: instAddrs["foo.from"], + }, + ), + ), }, []string{ `foo.from`, @@ -569,13 +575,13 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.bar[0].foo.to"].UniqueKey(): { + 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"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `module.bar[0].foo.to`, @@ -606,17 +612,17 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - instAddrs["module.boo.foo.from"].UniqueKey(): { + Changes: addrs.MakeMap( + addrs.MakeMapElem(instAddrs["module.boo.foo.from"], MoveSuccess{ instAddrs["foo.from"], instAddrs["module.boo.foo.from"], - }, - instAddrs["module.boo.foo.to"].UniqueKey(): { + }), + addrs.MakeMapElem(instAddrs["module.boo.foo.to"], MoveSuccess{ instAddrs["module.bar[0].foo.to"], instAddrs["module.boo.foo.to"], - }, - }, - Blocked: map[addrs.UniqueKey]MoveBlocked{}, + }), + ), + Blocked: emptyResults.Blocked, }, []string{ `module.boo.foo.from`, @@ -648,19 +654,21 @@ func TestApplyMoves(t *testing.T) { ) }), MoveResults{ - Changes: map[addrs.UniqueKey]MoveSuccess{ - - instAddrs["module.boo.foo.from"].UniqueKey(): { + Changes: addrs.MakeMap( + addrs.MakeMapElem(instAddrs["module.boo.foo.from"], MoveSuccess{ instAddrs["module.bar[0].foo.from"], instAddrs["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(), - }, - }, + }), + ), + Blocked: addrs.MakeMap( + addrs.MakeMapElem[addrs.AbsMoveable]( + instAddrs["foo.from"].ContainingResource(), + MoveBlocked{ + Actual: instAddrs["foo.from"].ContainingResource(), + Wanted: instAddrs["module.boo.foo.from"].ContainingResource(), + }, + ), + ), }, []string{ `foo.from`, diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index 13f2e50f9590..7d00686aecea 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -44,8 +44,8 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts Other addrs.AbsMoveable StmtRange tfdiags.SourceRange } - stmtFrom := map[addrs.UniqueKey]AbsMoveEndpoint{} - stmtTo := map[addrs.UniqueKey]AbsMoveEndpoint{} + stmtFrom := addrs.MakeMap[addrs.AbsMoveable, AbsMoveEndpoint]() + stmtTo := addrs.MakeMap[addrs.AbsMoveable, AbsMoveEndpoint]() for _, stmt := range stmts { // Earlier code that constructs MoveStatement values should ensure that @@ -89,10 +89,8 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts absFrom := stmt.From.InModuleInstance(modInst) absTo := stmt.To.InModuleInstance(modInst) - fromKey := absFrom.UniqueKey() - toKey := absTo.UniqueKey() - if fromKey == toKey { + if addrs.Equivalent(absFrom, absTo) { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Redundant move statement", @@ -149,8 +147,8 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts } // There can only be one destination for each source address. - if existing, exists := stmtFrom[fromKey]; exists { - if existing.Other.UniqueKey() != toKey { + if existing, exists := stmtFrom.GetOk(absFrom); exists { + if !addrs.Equivalent(existing.Other, absTo) { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Ambiguous move statements", @@ -163,15 +161,15 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts }) } } else { - stmtFrom[fromKey] = AbsMoveEndpoint{ + stmtFrom.Put(absFrom, AbsMoveEndpoint{ Other: absTo, StmtRange: stmt.DeclRange, - } + }) } // There can only be one source for each destination address. - if existing, exists := stmtTo[toKey]; exists { - if existing.Other.UniqueKey() != fromKey { + if existing, exists := stmtTo.GetOk(absTo); exists { + if !addrs.Equivalent(existing.Other, absFrom) { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Ambiguous move statements", @@ -184,10 +182,10 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts }) } } else { - stmtTo[toKey] = AbsMoveEndpoint{ + stmtTo.Put(absTo, AbsMoveEndpoint{ Other: absFrom, StmtRange: stmt.DeclRange, - } + }) } // Resource types must match. diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 3364bca0190b..c7a7c207c196 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -410,7 +410,7 @@ func (c *Context) prePlanVerifyTargetedMoves(moveResults refactoring.MoveResults var diags tfdiags.Diagnostics var excluded []addrs.AbsResourceInstance - for _, result := range moveResults.Changes { + for _, result := range moveResults.Changes.Values() { fromMatchesTarget := false toMatchesTarget := false for _, targetAddr := range targets { @@ -522,7 +522,7 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, o } diags = diags.Append(moveValidateDiags) // might just contain warnings - if len(moveResults.Blocked) > 0 && !diags.HasErrors() { + if moveResults.Blocked.Len() > 0 && !diags.HasErrors() { // If we had blocked moves and we're not going to be returning errors // then we'll report the blockers as a warning. We do this only in the // absense of errors because invalid move statements might well be @@ -592,7 +592,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, 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.Changes) == 0 { + if newState.ManagedResourcesEqual(oldState) && moves.Changes.Len() == 0 { // Nothing to do, because we only detect and report drift for managed // resource instances. return nil, diags @@ -624,7 +624,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.Changes[addr.UniqueKey()]; ok { + if move, ok := moves.Changes.GetOk(addr); ok { prevRunAddr = move.From } @@ -749,13 +749,13 @@ func (c *Context) PlanGraphForUI(config *configs.Config, prevRunState *states.St } func blockedMovesWarningDiag(results refactoring.MoveResults) tfdiags.Diagnostic { - if len(results.Blocked) < 1 { + if results.Blocked.Len() < 1 { // Caller should check first panic("request to render blocked moves warning without any blocked moves") } var itemsBuf bytes.Buffer - for _, blocked := range results.Blocked { + for _, blocked := range results.Blocked.Values() { fmt.Fprintf(&itemsBuf, "\n - %s could not move to %s", blocked.Actual, blocked.Wanted) }