config: Add support for lists to concat

[radeksimko]

The original intention was to satisfy @EvanKrall in this comment: #1495 (comment)

but it turns out that lists are generally not a good idea for these cases as there's no way to uniquely identify each member of the list => when any member of the array is changed, many others will be changed too, because unique ID = module.dns.dnsimple_record.foo.0 where 0 is not unique identifier and resource with that ID will change when you may not expect it to be changed.

In case of DNS records, it's a problem, because it will iterate over these resources 0..5 and try adding records that are already there.

It will only work well when changing/removing members from the end of the final list.

Support for maps in variables + zip(keys, vals) would be probably the way forward for case like this one.

So to sum it up, it probably won't solve that particular example, but I was thinking it may be useful elsewhere.

Example

${concat(aws_instance.db.*.tags.Name, aws_instance.web.*.tags.Name)}

[mitchellh]

Thanks! So we talked about this during our PR review Hangout and we decided that a better name for this would be concat. However, as you probably saw, concat is already taken. But, as a history lesson which you might be aware of but just putting here for the future: concat exists because string concatenation with interpolations previously wasn't possible. For example, you used to not be able to do: ${function("foo ${bar}", arg)}. You'd have to do ${function(concat("foo ", bar), arg))}. Well, you don't have to do this anymore. So we kept around concat for backwards compatibility.

Because of that, I think what we should do is actually rename this to concat, and make concat with two strings just do the old string concatenation. This way the backwards compatibility is preserved while the new functionality is there.

To deprecate things further, I think we should jus remove concat from the docs (the old one), and only document the new behavior. That way, only legacy users even know the old behavior exists.

And one day we can put a warning in there.

[radeksimko]

@mitchellh Changed as requested.

I also had to deal with cases like string+list - that can be an error or simply a list (i.e. string being one of the members of a new list). I decided to convert that into a list, but I'm open for suggestions.

[lamdor]

I can verify that this solves my use case in #2113

[lamdor]

@radeksimko @mitchellh Any hope of getting this merged? I still have a use for it. Thanks.

[phinze]

@rubbish heh, me too! making my way down the PRs for 0.6 but will pick this one up next for ya.

[phinze]

LGTM - merging and I'll document the breaking change.

[lamdor]

Thanks @phinze @radeksimko

[jedineeper]

Sorry, wanted to confirm that this means that the previous functionality of merging strings has been removed from concat? Is there a new alternative?

[radeksimko]

@jedineeper

${var.first}${var.second}${var.third}

concat will work the same way if you pass raw strings though, I reckon we'll keep it backward compatible for a few versions until people notice and change the code relying on that behaviour.

[CloudSurgeon]

Ok. So, just upgraded to 0.7 and I can't figure out how to get this to work without concat:
ami = "${lookup(var.lt_amis, concat(var.aws_region,var.setup_level,var.system_version))}"

WHERE aws_region, setup_level, and system_version are all strings. This worked in 0.6X.

Perhaps I'm just being dense...

[CloudSurgeon]

Yep. Just being dense. figured it out.
ami = "${lookup(var.lt_amis, "${var.aws_region}${var.setup_level}${var.delphix_version}")}"

[ghost]

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.