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

Split of "" should be empty. Length of empty array should be 0 #2973

Merged
merged 1 commit into from
Oct 29, 2015

Conversation

bobtfish
Copy link
Contributor

In place we want to do: count = "${length(split(",", var.list_of_things))}" and in some cases have an empty list (i.e. ""), and therefore get 0 resources.

To do this, splitting the string "" needs to return an empty array (rather than an array with one item which is empty), and the length function needs to do the right thing if it sees an empty array.

@bobtfish bobtfish force-pushed the length_empty_split_zero branch 2 times, most recently from c6ca63a to ff8fb43 Compare August 11, 2015 16:35
@radeksimko
Copy link
Member

Hi Tom,
I was not testing your code thoroughly, but isn't this a duplicate of #2157 ?

@bobtfish
Copy link
Contributor Author

@radeksimko - no, or at least not entirely if you add just my test changes to your branch, then the tests fail, like so:

--- FAIL: TestInterpolateFuncLength (0.00s)
interpolate_funcs_test.go:585: 5: bad output for input: ${length(split(",", ""))}

    Output: "1"
    Expected: "0"
--- FAIL: TestInterpolateFuncSplit (0.00s)
interpolate_funcs_test.go:585: 1: bad output for input: ${split(",", "")}

    Output: "B780FFEC-B661-4EB8-9236-A01737AD98B6B780FFEC-B661-4EB8-9236-A01737AD98B6"
    Expected: "B780FFEC-B661-4EB8-9236-A01737AD98B6"
FAIL
FAIL    github.com/hashicorp/terraform/config   0.040s

@phinze
Copy link
Contributor

phinze commented Sep 14, 2015

I went back and forth on this as I worked on the StringList refactor.

https://twitter.com/phinze/status/614116280480739328

I think this sums it up nicely:

it's more consistent to return [""], but more useful in most situations to return [].

So I figured we'd toss in a compact or without func to allow us to implement it it the consistent way, but I'd willing to be persuaded in the other direction.

Seems like a tough trade off between "one more interp func" and POLA. Thoughts?

@thegedge
Copy link
Contributor

Although quite kludgy, I think one way to do this at the moment is to use a template_file that appends a comma to a string containing a comma-separated list, and do ${length(split(",", replace("/^,$/", "", template_file.foo.rendered))) - 1}.

My five cents: I think having split returning a unitary list for empty strings is expected for anyone coming from any (all?) other language, but it feels like it should have the alternative behaviour since we can't pass around lists "natively" (i.e., resorting to join/split). I'd lean towards having another interpolation function for now because it would lead to nicer configs, but would consider working towards better support for lists as variables/outputs which I think would alleviate the problems that led to this PR.

@apparentlymart
Copy link
Contributor

Now that #3239 is merged, and thus we have the compact function, does that address the original need here?

In other words, it should now be possible to write the original example as count = "${length(compact(split(",", var.list_of_things)))}" and get 0 when the list_of_things is the empty string.

@bobtfish
Copy link
Contributor Author

Sounds great to me.

I've added a test explicitly testing the combination of all 3 functions, as I didn't see that currently.

@phinze
Copy link
Contributor

phinze commented Oct 29, 2015

A PR that's iterated into a test addition. 💓 Thanks @bobtfish!

phinze added a commit that referenced this pull request Oct 29, 2015
Split of "" should be empty. Length of empty array should be 0
@phinze phinze merged commit c56245c into hashicorp:master Oct 29, 2015
@elblivion
Copy link
Contributor

🎉

omeid pushed a commit to omeid/terraform that referenced this pull request Mar 30, 2018
…userdata_validation

Port user_data length validation to aws_launch_configuration
omeid pushed a commit to omeid/terraform that referenced this pull request Mar 30, 2018
@ghost
Copy link

ghost commented Apr 30, 2020

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 Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants