Skip to content

Commit

Permalink
Merge pull request #29659 from hashicorp/jbardin/really-nested-within
Browse files Browse the repository at this point in the history 
refactoring: exhaustive NestedWithin checks
  • Loading branch information
@jbardin
jbardin authored Sep 27, 2021
2 parents a742d7e + cac1f5c commit 372814e
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 105 deletions.
39 changes: 36 additions & 3 deletions internal/refactoring/move_execute.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
Expand All @@ -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
Expand Down
206 changes: 104 additions & 102 deletions internal/refactoring/move_execute_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
instAddrs["module.bar[0].foo.to"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
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"],
&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.from"],
},
instAddrs["module.boo.foo.to"].UniqueKey(): {
instAddrs["module.bar[0].foo.to"],
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{},
},
*/

// 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(
[]string{
`module.boo.foo.from`,
`module.boo.foo.to`,
},
},

"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 {
Expand Down

0 comments on commit 372814e

Please sign in to comment.