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

Fix the logic for null[*] #416

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Conversation

remilapeyre
Copy link
Contributor

According to https://discuss.hashicorp.com/t/pattern-to-handle-optional-dynamic-blocks/2384/2
null[*] is defined and should return an empty tuple. The logic in
SplatExpr.Value() is slightly wrong thought as it returns early with a
dynamic value even when the value is actually known to be null.

The fix is easy, just check for null sources first, then for dynamic
values. I added a test for this in TestExpressionParseAndValue().

According to https://discuss.hashicorp.com/t/pattern-to-handle-optional-dynamic-blocks/2384/2
`null[*]` is defined and should return an empty tuple. The logic in
SplatExpr.Value() is slightly wrong thought as it returns early with a
dynamic value even when the value is actually known to be null.

The fix is easy, just check for null sources first, then for dynamic
values. I added a test for this in TestExpressionParseAndValue().
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

👍 thank you!

@apparentlymart
Copy link
Contributor

Thanks for working on this @remilapeyre!

Based on the discussion over in hashicorp/terraform#26746, the behavior change you've proposed seems correct and consistent with our usual sense of backward compatibility for HCL behaviors of this sort (the new result is no more unknown than the old). I'm going to merge this now.

Because null[*] is not a common language construct and since this has unfortunately narrowly missed an earlier HCL patch release today I'm not going to issue another one just for this one change. There are some other open pull requests currently awaiting review and so I expect at least one of those will prompt a new release before too long. With that said, this does mean that it won't be possible to incorporate this into Terraform immediately because Terraform typically tracks only tagged releases of libraries unless there's a high-urgency reason to do otherwise. We can bring it in to Terraform once it's available in a HCL release, though.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants