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

Resource value may be an object in self evaluation #23215

Merged
merged 2 commits into from
Oct 29, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 7 additions & 3 deletions lang/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,19 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl
// Self is an exception in that it must always resolve to a
// particular instance. We will still insert the full resource into
// the context below.
var hclDiags hcl.Diagnostics
// We should always have a valid self index by this point, but in
// the case of an error, self may end up as a cty.DynamicValue.
switch k := subj.Key.(type) {
case addrs.IntKey:
self = val.Index(cty.NumberIntVal(int64(k)))
self, hclDiags = hcl.Index(val, cty.NumberIntVal(int64(k)), ref.SourceRange.ToHCL().Ptr())
diags.Append(hclDiags)
case addrs.StringKey:
self = val.Index(cty.StringVal(string(k)))
self, hclDiags = hcl.Index(val, cty.StringVal(string(k)), ref.SourceRange.ToHCL().Ptr())
diags.Append(hclDiags)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pondering what happens if hcl.Index fails here.

It looks like it shouldn't happen because val should always be of a suitable type by the time we get here (or else we would've been panicking before anyway), so maybe not a big deal, but perhaps worth adding a comment to that effect anyway to assuage the concerns of future readers.

It looks like hcl.Index is consistent with the usual HCL convention that if it returns errors then it also returns an incomplete-but-valid placeholder value (cty.DynamicVal, in this case) so later code below will hopefully be okay with self being cty.DynamicVal and just short-circuit as normal and allow the error to be returned. Might therefore be okay to just add a comment here noting that we're not expecting this to return errors, but if it does then self will be cty.DynamicVal, in case that's helpful to a future maintainer.

Copy link
Member Author

Choose a reason for hiding this comment

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

That matches my thoughts here too. I'm actually not sure which is better now, since in the rare case of an internal error making it this far we panic with the exact location. I'm still leaning towards collecting the error and passing it along for user ergonomics, so I'll just add another comment for now.

default:
self = val
}

continue
}

Expand Down
38 changes: 38 additions & 0 deletions terraform/context_apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6137,6 +6137,44 @@ func TestContext2Apply_provisionerExplicitSelfRef(t *testing.T) {
}
}

func TestContext2Apply_provisionerForEachSelfRef(t *testing.T) {
m := testModule(t, "apply-provisioner-for-each-self")
p := testProvider("aws")
pr := testProvisioner()
p.ApplyFn = testApplyFn
p.DiffFn = testDiffFn

pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error {
val, ok := c.Config["command"]
if !ok {
t.Fatalf("bad value for command: %v %#v", val, c)
}

return nil
}

ctx := testContext2(t, &ContextOpts{
Config: m,
ProviderResolver: providers.ResolverFixed(
map[string]providers.Factory{
"aws": testProviderFuncFixed(p),
},
),
Provisioners: map[string]ProvisionerFactory{
"shell": testProvisionerFuncFixed(pr),
},
})

if _, diags := ctx.Plan(); diags.HasErrors() {
t.Fatalf("plan errors: %s", diags.Err())
}

_, diags := ctx.Apply()
if diags.HasErrors() {
t.Fatalf("diags: %s", diags.Err())
}
}

// Provisioner should NOT run on a diff, only create
func TestContext2Apply_Provisioner_Diff(t *testing.T) {
m := testModule(t, "apply-provisioner-diff")
Expand Down
8 changes: 8 additions & 0 deletions terraform/testdata/apply-provisioner-for-each-self/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
resource "aws_instance" "foo" {
for_each = toset(["a", "b", "c"])
foo = "number ${each.value}"

provisioner "shell" {
command = "${self.foo}"
}
}