Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Module depends_on #25005

Merged
merged 4 commits into from
May 29, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions configs/module_call.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 0 additions & 1 deletion configs/module_call_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.`,
})

Expand Down
60 changes: 60 additions & 0 deletions terraform/context_apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Comment on lines +11173 to +11174
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something obvious, but I don't understand this comment. Why does re-planning after apply ensure that the data source isn't re-read?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the data source is read during plan (which is a new ability from #24904), it would show in the plan as a read operation. There should be nothing left to apply at this point, so anything in the plan should be NoOp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see now, thanks!

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)
}
}
}
20 changes: 18 additions & 2 deletions terraform/node_module_expand.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package terraform

import (
"fmt"
"log"

"github.com/hashicorp/terraform/addrs"
"github.com/hashicorp/terraform/configs"
Expand Down Expand Up @@ -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
Expand All @@ -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)
}
Expand Down
32 changes: 32 additions & 0 deletions terraform/testdata/apply-module-depends-on/main.tf
Original file line number Diff line number Diff line change
@@ -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
}
10 changes: 10 additions & 0 deletions terraform/testdata/apply-module-depends-on/moda/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
resource "test_instance" "a" {
num = 5
}

data "test_data_source" "a" {
}

output "out" {
value = data.test_data_source.a.id
}
10 changes: 10 additions & 0 deletions terraform/testdata/apply-module-depends-on/modb/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
resource "test_instance" "b" {
num = 2
}

data "test_data_source" "b" {
}

output "out" {
value = test_instance.b.id
}