From 1b8fb4083a129c04bbbb835ed1c504bb589edccf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 28 Apr 2020 17:14:15 -0400 Subject: [PATCH 1/4] allow depends_on in module call --- configs/module_call.go | 8 -------- configs/module_call_test.go | 1 - 2 files changed, 9 deletions(-) diff --git a/configs/module_call.go b/configs/module_call.go index cf6d1acbd..25f048136 100644 --- a/configs/module_call.go +++ b/configs/module_call.go @@ -87,14 +87,6 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno deps, depsDiags := decodeDependsOn(attr) diags = append(diags, depsDiags...) mc.DependsOn = append(mc.DependsOn, deps...) - - // We currently parse this, but don't yet do anything with it. - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Reserved argument name in module block", - Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), - Subject: &attr.NameRange, - }) } if attr, exists := content.Attributes["providers"]; exists { diff --git a/configs/module_call_test.go b/configs/module_call_test.go index d1164635e..79ad89e62 100644 --- a/configs/module_call_test.go +++ b/configs/module_call_test.go @@ -20,7 +20,6 @@ func TestLoadModuleCall(t *testing.T) { file, diags := parser.LoadConfigFile("module-calls.tf") assertExactDiagnostics(t, diags, []string{ - `module-calls.tf:22,3-13: Reserved argument name in module block; The name "depends_on" is reserved for use in a future version of Terraform.`, `module-calls.tf:20,3-11: Invalid combination of "count" and "for_each"; The "count" and "for_each" meta-arguments are mutually-exclusive, only one should be used to be explicit about the number of resources to be created.`, }) From e1f607c9eb510eaa8b23709be3af7d6e9a406a92 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 28 Apr 2020 17:15:40 -0400 Subject: [PATCH 2/4] add depends_on references for modules to graph Connect references from depends_on in modules calls. This will "just work" for a lot of cases, but data sources will be read too early in the case where they require the dependencies to be created. While data sources will be properly ordered behind the module head node, there is nothing preventing them from being being evaluated during refresh. --- terraform/node_module_expand.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 0f8a070a3..08961e9e7 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -2,6 +2,7 @@ package terraform import ( "fmt" + "log" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" @@ -52,6 +53,19 @@ func (n *nodeExpandModule) References() []*addrs.Reference { return nil } + for _, traversal := range n.ModuleCall.DependsOn { + ref, diags := addrs.ParseRef(traversal) + if diags.HasErrors() { + // We ignore this here, because this isn't a suitable place to return + // errors. This situation should be caught and rejected during + // validation. + log.Printf("[ERROR] Can't parse %#v from depends_on as reference: %s", traversal, diags.Err()) + continue + } + + refs = append(refs, ref) + } + // Expansion only uses the count and for_each expressions, so this // particular graph node only refers to those. // Individual variable values in the module call definition might also @@ -64,10 +78,12 @@ func (n *nodeExpandModule) References() []*addrs.Reference { // child module instances we might expand to during our evaluation. if n.ModuleCall.Count != nil { - refs, _ = lang.ReferencesInExpr(n.ModuleCall.Count) + countRefs, _ := lang.ReferencesInExpr(n.ModuleCall.Count) + refs = append(refs, countRefs...) } if n.ModuleCall.ForEach != nil { - refs, _ = lang.ReferencesInExpr(n.ModuleCall.ForEach) + forEachRefs, _ := lang.ReferencesInExpr(n.ModuleCall.ForEach) + refs = append(refs, forEachRefs...) } return appendResourceDestroyReferences(refs) } From 14ef51bfcd792d07411d5f1d0d9d040a28f522dd Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 20 May 2020 14:46:30 -0400 Subject: [PATCH 3/4] module depends_on test verify a chain of depends_on references through modules execute in the correct order --- terraform/context_apply_test.go | 60 +++++++++++++++++++ .../testdata/apply-module-depends-on/main.tf | 32 ++++++++++ .../apply-module-depends-on/moda/main.tf | 10 ++++ .../apply-module-depends-on/modb/main.tf | 10 ++++ 4 files changed, 112 insertions(+) create mode 100644 terraform/testdata/apply-module-depends-on/main.tf create mode 100644 terraform/testdata/apply-module-depends-on/moda/main.tf create mode 100644 terraform/testdata/apply-module-depends-on/modb/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index af22fc9b4..aed29cdc1 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11122,3 +11122,63 @@ resource "aws_instance" "cbd" { t.Fatal("aws_instance.foo should also be create_before_destroy") } } + +func TestContext2Apply_moduleDependsOn(t *testing.T) { + m := testModule(t, "apply-module-depends-on") + + p := testProvider("test") + p.ReadDataSourceResponse = providers.ReadDataSourceResponse{ + State: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("data"), + "foo": cty.NullVal(cty.String), + }), + } + p.DiffFn = testDiffFn + + // each instance being applied should happen in sequential order + applied := int64(0) + + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) { + state := req.PlannedState.AsValueMap() + num, _ := state["num"].AsBigFloat().Float64() + ord := int64(num) + if !atomic.CompareAndSwapInt64(&applied, ord-1, ord) { + actual := atomic.LoadInt64(&applied) + resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("instance %d was applied after %d", ord, actual)) + } + + state["id"] = cty.StringVal(fmt.Sprintf("test_%d", ord)) + resp.NewState = cty.ObjectVal(state) + + return resp + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + // run the plan again to ensure that data sources are not going to be re-read + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + for _, res := range plan.Changes.Resources { + if res.Action != plans.NoOp { + t.Fatalf("expected NoOp, got %s for %s", res.Action, res.Addr) + } + } +} diff --git a/terraform/testdata/apply-module-depends-on/main.tf b/terraform/testdata/apply-module-depends-on/main.tf new file mode 100644 index 000000000..9f7102d53 --- /dev/null +++ b/terraform/testdata/apply-module-depends-on/main.tf @@ -0,0 +1,32 @@ +module "moda" { + source = "./moda" + depends_on = [test_instance.a, module.modb] +} + +resource "test_instance" "a" { + depends_on = [module.modb] + num = 4 + foo = test_instance.aa.id +} + +resource "test_instance" "aa" { + num = 3 + foo = module.modb.out +} + +module "modb" { + source = "./modb" + depends_on = [test_instance.b] +} + +resource "test_instance" "b" { + num = 1 +} + +output "moda_data" { + value = module.moda.out +} + +output "modb_resource" { + value = module.modb.out +} diff --git a/terraform/testdata/apply-module-depends-on/moda/main.tf b/terraform/testdata/apply-module-depends-on/moda/main.tf new file mode 100644 index 000000000..914e545c8 --- /dev/null +++ b/terraform/testdata/apply-module-depends-on/moda/main.tf @@ -0,0 +1,10 @@ +resource "test_instance" "a" { + num = 5 +} + +data "test_data_source" "a" { +} + +output "out" { + value = data.test_data_source.a.id +} diff --git a/terraform/testdata/apply-module-depends-on/modb/main.tf b/terraform/testdata/apply-module-depends-on/modb/main.tf new file mode 100644 index 000000000..17192b039 --- /dev/null +++ b/terraform/testdata/apply-module-depends-on/modb/main.tf @@ -0,0 +1,10 @@ +resource "test_instance" "b" { + num = 2 +} + +data "test_data_source" "b" { +} + +output "out" { + value = test_instance.b.id +} From d2466fab3d6ac9a8e0f1aaf7c1c781f5af901d8b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 27 May 2020 14:24:58 -0400 Subject: [PATCH 4/4] add module self-reference test --- terraform/context_apply_test.go | 70 +++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index aed29cdc1..93b790834 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11182,3 +11182,73 @@ func TestContext2Apply_moduleDependsOn(t *testing.T) { } } } + +func TestContext2Apply_moduleSelfReference(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +module "test" { + source = "./test" + + a = module.test.b +} + +output "c" { + value = module.test.c +} +`, + "test/main.tf": ` +variable "a" {} + +resource "test_instance" "test" { +} + +output "b" { + value = test_instance.test.id +} + +output "c" { + value = var.a +}`}) + + p := testProvider("test") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + ctx = testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + Destroy: true, + }) + + _, diags = ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + state, diags := ctx.Apply() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + if !state.Empty() { + t.Fatal("expected empty state, got:", state) + } +}