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

config: Add length to built-in functions #1495

Merged
merged 1 commit into from
Apr 15, 2015

Conversation

radeksimko
Copy link
Member

Related issue: #1389

I would really welcome some feedback regarding expected use-cases of this function - i.e. see the test cases and input/output.

I've taken certain assumptions about edge cases like empty members and strings generally.

  1. Some people may expect ${length(split(",one,"))} to return 3, but I can't think of any real use case for empty strings here, so ${length(split(",one,"))} = 1
  2. Some people may expect ${length("string")} to return 6 instead of 1. I could try and search for InterpSplitDelim and if it's not there, just treat the input as string. done.

Once you'll be happy with the behaviour, I can add some docs too (and describe the behaviour there as well).

I'm not entirely sure @mitchellh will want to see this implemented before a real support for arrays.

@radeksimko radeksimko changed the title length added to built-in functions config: Add length to built-in functions Apr 11, 2015
@EvanKrall
Copy link
Contributor

I agree that empty strings are often not desired, but I think your length function should count them regardless. I think a common use case will be something like:

resource "some" "thing" {
  count = "${length(split(",", var.foo_list))}"
  foo = "${element(split(",", var.foo_list), count.index)}"
}

In which case, setting foo_list to ",something," would never create an object with foo = "something"

Maybe it would be better to have another function that cleans empty strings from a list? I could have used that the other day.

@radeksimko
Copy link
Member Author

In which case, setting foo_list to ",something," would never create an object with foo = "something"

Could you provide any real example with real resources/options (replacing "some" and "foo"), so it's easier to imagine the use case?

@nevir
Copy link
Contributor

nevir commented Apr 11, 2015

For that split example, it seems like split should be the one to ignore empty strings

@EvanKrall
Copy link
Contributor

A contrived example might be if you had a module for managing your DNS:

var "hostnames" {}
var "ips" {}

resource "dnsimple_record" "foo" {
  count = "${length(split(" ", var.hostnames))}"
  domain = "mydomain.ceo"
  type = "A"
  name = "${element(split(" ", var.hostnames), count.index)}"
  value = "${element(split(" ", var.ips), count.index)}"
}

I might call this module like so:

module "dns" {
  source = "./modules/dns"
  hostnames = "${join(" ", aws_instance.db.*.tags.Name)} ${join(" ", aws_instance.log.*.tags.Name)} ${join(" ", aws_instance.web.*.tags.Name)}"
  ips = "${join(" ", aws_instance.db.*.private_ip)} ${join(" ", aws_instance.log.*.private_ip)} ${join(" ", aws_instance.web.*.private_ip)}"
}

Let's say count = 0 on aws_instance.db for some reason, and count = 3 on web and log. Then split(" ", var.hostnames) is going to give us a 7-element list with an empty string at the beginning, e.g. ["", "log1", "log2", "log3", "web1", "web2", "web3"]

Now your length function will return 6 because it's ignoring empty strings; so I'll end up with 6 DNS records: one where name and value are both empty, and then the first five of my boxes.

I'd argue that's even more confusing than having 7 dnsimple_record resources, with one having an empty value.

This is not to mention all the cases where you actually might want empty strings.

@c4milo
Copy link
Contributor

c4milo commented Apr 11, 2015

Most of my use cases are related to get the length of a list not a string.

@radeksimko radeksimko force-pushed the length-func branch 2 times, most recently from 437b508 to 96de968 Compare April 12, 2015 14:19
@radeksimko
Copy link
Member Author

@EvanKrall Thank you for the example.

It's worth mentioning that you can't actually reference tags (just some weirdness with modules, you actually can...), so I tested the example with instance IDs instead and I can confirm that it is an edge case that should be handled properly.

The question I was asking originally was "Is there any use case for empty strings?".

Your example is merely pointing to inconsistency between split and length where split does not ignore empty strings. While I agree that it is very confusing, I'm rather tempted to modify split so it ignores empty strings too instead of making length respect it.

You probably wouldn't want to see empty strings in your example anyway - what's the point of creating DNS records pointing to an empty string? (putting aside the fact that API won't allow you to do that)

@radeksimko
Copy link
Member Author

In fact the problem is not just a whitespace, I believe that any character which is used as a delimiter in split & join should IMO be trimmed off from both ends.

I can implement that with no issues, but we need to make a decision about length(string) VS length(list), because it will effectively convert a "list" to a real string by trimming these things off, therefore ${length(split(",", "foo,"))} => 3 and ${length(split(",", ",ee,"))} => 2

@mitchellh
Copy link
Contributor

I like this! For "blank" things, I think we should just have a separate function compact to remove them from a list. I believe length should not change behavior depending on blank/non-blank things.

For strings: I think in the future we need to improve typing to support first class lists (always planned), and use that to determine the behavior. I think for now the behavior in this PR is fine.

@radeksimko
Copy link
Member Author

I believe length should not change behavior depending on blank/non-blank things.

OK, thanks for the input, I will update the PR accordingly then.

@radeksimko
Copy link
Member Author

PR updated accordingly, please let me know if there's anything else you'd change or if it's good to go.

@phinze
Copy link
Contributor

phinze commented Apr 15, 2015

This is looking great - thanks! Merging now. Think you could follow up with a little docs PR?

phinze added a commit that referenced this pull request Apr 15, 2015
config: Add length to built-in functions
@phinze phinze merged commit 5874fa0 into hashicorp:master Apr 15, 2015
@radeksimko radeksimko deleted the length-func branch April 15, 2015 17:49
@radeksimko
Copy link
Member Author

@phinze Docs added in #1542

@ghost
Copy link

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

Successfully merging this pull request may close these issues.

6 participants