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.
This commit is contained in:
Martin Atkins 2022-06-13 11:56:53 -07:00
parent eb2374070f
commit dc5964f8a3
5 changed files with 142 additions and 135 deletions

View File

@ -7,10 +7,10 @@ package addrs
// by ranging over the map values, but making direct modifications could
// potentially make the set data invalid or inconsistent, leading to undefined
// behavior elsewhere.
type Set[T UniqueKeyer] map[UniqueKey]UniqueKeyer
type Set[T UniqueKeyer] map[UniqueKey]T
// Has returns true if and only if the set includes the given address.
func (s Set[T]) Has(addr UniqueKeyer) bool {
func (s Set[T]) Has(addr T) bool {
_, exists := s[addr.UniqueKey()]
return exists
}
@ -18,13 +18,13 @@ func (s Set[T]) Has(addr UniqueKeyer) bool {
// Add inserts the given address into the set, if not already present. If
// an equivalent address is already in the set, this replaces that address
// with the new value.
func (s Set[T]) Add(addr UniqueKeyer) {
func (s Set[T]) Add(addr T) {
s[addr.UniqueKey()] = addr
}
// Remove deletes the given address from the set, if present. If not present,
// this is a no-op.
func (s Set[T]) Remove(addr UniqueKeyer) {
func (s Set[T]) Remove(addr T) {
delete(s, addr.UniqueKey())
}

View File

@ -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
}

View File

@ -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`,

View File

@ -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.

View File

@ -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)
}