From cc964e6b0b995b37eadd9d7118a9af6bc93d5f3f Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 26 Sep 2022 11:59:37 -0700 Subject: [PATCH] core: Document that TransformRoot must produce coalescable node 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. --- internal/terraform/context_plan2_test.go | 24 +++++++++++++++++++++--- internal/terraform/transform_root.go | 14 ++++++++++++-- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 6cc8eb5a1cec..d96da31cb13d 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -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 @@ -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 { diff --git a/internal/terraform/transform_root.go b/internal/terraform/transform_root.go index 27804ff0248c..b22ef11fc0f7 100644 --- a/internal/terraform/transform_root.go +++ b/internal/terraform/transform_root.go @@ -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