Skip to content

Commit

Permalink
core: Document that TransformRoot must produce coalescable node
Browse files Browse the repository at this point in the history
We use a non-pointer value for this particular node, which means that
there can never be two root nodes in the same graph: the graph
implementation will just coalesce them together when a second one is added.

Our resource expansion code is relying on that coalescing so that it can
subsume together multiple graphs for different modules instances into a
single mega-graph with all instances across all module instances, with
any root nodes coalescing together to produce a single root.

This also updates one of the context tests that exercises resource
expansion so that it will generate multiple resource instance nodes per
module and thus potentially have multiple roots to coalesce together.
However, we aren't currently explicitly validating the return values from
DynamicExpand and so this test doesn't actually fail if the coalescing
doesn't happen. We may choose to validate the DynamicExpand result in a
later commit in order to make it more obvious if future modifications fail
to uphold this invariant.
  • Loading branch information
apparentlymart committed Sep 26, 2022
1 parent 2e177cd commit cc964e6
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 5 deletions.
24 changes: 21 additions & 3 deletions internal/terraform/context_plan2_test.go
Expand Up @@ -453,7 +453,21 @@ func TestContext2Plan_resourceChecksInExpandedModule(t *testing.T) {
a = "a"
}
resource "test" "test" {
resource "test" "test1" {
lifecycle {
postcondition {
# It doesn't matter what this checks as long as it
# passes, because if we don't handle expansion properly
# then we'll crash before we even get to evaluating this.
condition = local.a == local.a
error_message = "Postcondition failed."
}
}
}
resource "test" "test2" {
count = 2
lifecycle {
postcondition {
# It doesn't matter what this checks as long as it
Expand All @@ -478,8 +492,12 @@ func TestContext2Plan_resourceChecksInExpandedModule(t *testing.T) {
assertNoErrors(t, diags)

resourceInsts := []addrs.AbsResourceInstance{
mustResourceInstanceAddr("module.child[0].test.test"),
mustResourceInstanceAddr("module.child[1].test.test"),
mustResourceInstanceAddr("module.child[0].test.test1"),
mustResourceInstanceAddr("module.child[0].test.test2[0]"),
mustResourceInstanceAddr("module.child[0].test.test2[1]"),
mustResourceInstanceAddr("module.child[1].test.test1"),
mustResourceInstanceAddr("module.child[1].test.test2[0]"),
mustResourceInstanceAddr("module.child[1].test.test2[1]"),
}

for _, instAddr := range resourceInsts {
Expand Down
14 changes: 12 additions & 2 deletions internal/terraform/transform_root.go
Expand Up @@ -15,11 +15,21 @@ func (t *RootTransformer) Transform(g *Graph) error {
return nil
}

// Add a root
// We intentionally add a graphNodeRoot value -- rather than a pointer to
// one -- so that all root nodes will coalesce together if two graphs
// are merged. Each distinct node value can only be in a graph once,
// so adding another graphNodeRoot value to the same graph later will
// be a no-op and all of the edges from root nodes will coalesce together
// under Graph.Subsume.
//
// It's important to retain this coalescing guarantee under future
// maintenence.
var root graphNodeRoot
g.Add(root)

// Connect the root to all the edges that need it
// We initially make the root node depend on every node except itself.
// If the caller subsequently runs transitive reduction on the graph then
// it's typical for some of these edges to then be removed.
for _, v := range g.Vertices() {
if v == root {
continue
Expand Down

0 comments on commit cc964e6

Please sign in to comment.