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

coalescelist behavior with all empty lists changed in v0.12.0 #21370

Closed
apparentlymart opened this issue May 20, 2019 · 4 comments
Closed

coalescelist behavior with all empty lists changed in v0.12.0 #21370

apparentlymart opened this issue May 20, 2019 · 4 comments
Milestone

Comments

@apparentlymart
Copy link
Contributor

In prior versions of Terraform, coalescelist with all of the arguments being empty lists caused it to return an empty list.

In Terraform v0.12.0, that behavior has changed to be an error instead:

Error: Error in function call

  on .terraform/modules/vpc/terraform-aws-modules-terraform-aws-vpc-c0c3b52/outputs.tf line 204, in output "redshift_route_table_ids":
 204:   value       = [coalescelist(aws_route_table.redshift.*.id, aws_route_table.private.*.id)]
    |----------------
    | aws_route_table.private is empty tuple
    | aws_route_table.redshift is empty tuple

Call to function "coalescelist" failed: no non-null arguments.

While this new behavior makes sense by the direct definition of the function -- it's supposed to return the first non-empty list, and there isn't one to return -- there now isn't a concise way to say "return the first of these that isn't empty or otherwise return an empty list", which comes up from time to time.

This error situation was added to make this situation more obvious since we assumed that such usage would be user error that could otherwise lead to hard-to-debug incorrect behavior. However, there clearly are use-cases for it, so I think we'll need to either restore the old behavior as-is or provide some way to explicitly opt in to the old behavior.

@mildwonkey
Copy link
Contributor

I (personally) believe that explicitly adding a list with an empty string to coalescelist when the inputs may be null is reasonable, and we can update the documentation with such.

In terraform 0.13, coalesce with entirely empty strings returned an empty string. I think this is also reasonable behavior from a user's perspective, even if it feels inconsistent. I believe this is the original behavior from go-cty's coalesce function but have not yet verified.

@apparentlymart
Copy link
Contributor Author

apparentlymart commented Jun 1, 2019

The cty implementation of coalesce was modeled after the SQL COALESCE function rather than the function as defined in earlier versions of Terraform, so its main difference is that it only skips nulls, and would happily return an empty string if one were there because empty strings are not null.

We retained a Terraform-specific implementation in Terraform 0.12.0 so we wouldn't change the behavior of something like coalesce("", "a") to return empty string, which would've been a subtle breaking change relative to Terraform 0.11.

I believe (though I can't say for certain) that Terraform 0.11's coalesce was also somewhat modeled on the SQL function, but with the definition changed to use empty string instead of null because Terraform 0.11 and prior had no concept of null. But it was still self-consistent with that definition, because it would return an empty string if all of the given strings were empty.

coalescelist doesn't have such a strong connection to any existing thing in another language as far as I know, so we have some more freedom there to define the behavior as we see fit and only have our own language's consistency to worry about. I did like the idea of making all-empty be an error originally because it felt like that was likely to be a mistake that would cause subtle misbehavior. Given that the module I was using as an example has found a reasonably concise alternative formulation that is more explicit, I feel better about leaving it as an error now.

My current feeling on this is to leave both of these returning errors now, since the problem I originally anticipated here doesn't seem to have been problematic in practice and we've seen no reports of problems with coalesce (as opposed to coalescelist) yet. I think I was being too cautious/pessimistic in filing this originally. With that said, I'm going to close this one out for the moment, pending any direct feedback that might give us some insight into additional use-cases affected by this.

@dermyrddin
Copy link

Unfortunately this break some setups of AWS ec2-instance module: terraform-aws-modules/terraform-aws-ec2-instance#104

@ghost
Copy link

ghost commented Jul 25, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants