From 03edffd1ad7e866a0a1960ed181e4f96f0f74fe6 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 13 Jun 2022 11:56:53 -0700 Subject: [PATCH] refactoring: Use addrs.Map for maps with addresses as keys We introduced the addrs.UniqueKey and addrs.UniqueKeyer mechanics as part of implementing the ValidateMoves and ApplyMoves functions, as a way to better encapsulate the solution to the problem that lots of our address types aren't comparable and so cannot be used directly as map keys. However, exposing addrs.UniqueKey handling directly in the logic adds various noise to the algorithms and, in particular, obscures the fact that MoveResults.Changes and MoveResult.Blocked both have different map key types. Here then we'll use the new addrs.Map helper type, which encapsulates the idea of a map from an addrs.UniqueKeyer type to an arbitrary value type, using the unique keys as the map keys internally. This does unfortunately mean that we lose the conventional Go map access syntax and have to use a method-based API instead, but I (subjectively) think that's an okay compromise in return for avoiding the need to keep track inline of which addrs.UniqueKey values correspond with which real addresses. This is intended as an entirely-mechanical change, with equivalent behavior to what it replaced. If anything here is doing something materially different than what it replaced then that's a mistake. --- internal/refactoring/move_execute.go | 35 ++-- internal/refactoring/move_execute_test.go | 198 +++++++++++----------- internal/refactoring/move_validate.go | 24 ++- internal/terraform/context_plan.go | 12 +- 4 files changed, 138 insertions(+), 131 deletions(-) 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) }