core: Support native lists in Terraform configuration #6322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
As an aside - some of this was previously merged prior to the work done to remove flat-maps for map variables and get them to use native lists. Those commits were backed out during rebase of the cc @phinze, @mitchellh. |
|
Starting to dig into this - looking good so far! Noting that |
|
Found a crasher: variable "tagsmap" {
default = {
some = "tags"
cool = "beans"
}
}
resource "aws_vpc" "foo" {
cidr_block = "10.0.1.0/24"
tags = "${var.tagsmap}"
} |
|
Interesting. We should definitely support this. |
jen20
force-pushed
the
dev-0.7
branch
2 times, most recently
from
4046979 to
c119522
Compare
| @@ -80,6 +84,14 @@ func (n *EvalTypeCheckVariable) Eval(ctx EvalContext) (interface{}, error) { | |||
| return nil, fmt.Errorf("variable %s%s should be type %s, got %T", | |||
| name, modulePathDescription, declaredType.Printable(), proposedValue) | |||
| } | |||
There was a problem hiding this comment.
Just caught this error as a user when I didn't declare a type on a module variable and then passed in a map:
❯ tf plan
Error configuring: 1 error(s) occurred:
* variable tags in module mod should be type string, got map[string]interface {}
If I weren't a Go developer this would probably scare me. 😄
Happy to wait for error message twiddling for a subsequent PR though.
|
@jen20 this is looking great! LGTM for merge There are two things I think we should revisit in follow-on PRs:
|
|
@phinze Agreed on both counts. I'll address these in follow-up commits. For now I'll rebase this onto dev-0.7 (and update dev-0.7 onto the current master) and get it merged! |
This commit adds support for native list variables and outputs, building
up on the previous change to state. Interpolation functions now return
native lists in preference to StringList.
List variables are defined like this:
variable "test" {
# This can also be inferred
type = "list"
default = ["Hello", "World"]
}
output "test_out" {
value = "${var.a_list}"
}
This results in the following state:
```
...
"outputs": {
"test_out": [
"hello",
"world"
]
},
...
```
And the result of terraform output is as follows:
```
$ terraform output
test_out = [
hello
world
]
```
Using the output name, an xargs-friendly representation is output:
```
$ terraform output test_out
hello
world
```
The output command also supports indexing into the list (with
appropriate range checking and no wrapping):
```
$ terraform output test_out 1
world
```
Along with maps, list outputs from one module may be passed as variables
into another, removing the need for the `join(",", var.list_as_string)`
and `split(",", var.list_as_string)` which was previously necessary in
Terraform configuration.
This commit also updates the tests and implementations of built-in
interpolation functions to take and return native lists where
appropriate.
A backwards compatibility note: previously the concat interpolation
function was capable of concatenating either strings or lists. The
strings use case was deprectated a long time ago but still remained.
Because we cannot return `ast.TypeAny` from an interpolation function,
this use case is no longer supported for strings - `concat` is only
capable of concatenating lists. This should not be a huge issue - the
type checker picks up incorrect parameters, and the native HIL string
concatenation - or the `join` function - can be used to replicate the
missing behaviour.
Much celebration may now ensue! ♪┏(°.°)┛┗(°.°)┓┗(°.°)┛┏(°.°)┓ ♪
This adds a test and the support necessary to read from native maps
passed as variables via interpolation - for example:
```
resource ...... {
mapValue = "${var.map}"
}
```
We also add support for interpolating maps from the flat-mapped resource
config, which is necessary to support assignment of computed maps, which
is now valid.
Unfortunately there is no good way to distinguish between a list and a
map in the flatmap. In lieu of changing that representation (which is
risky), we assume that if all the keys are numeric, this is intended to
be a list, and if not it is intended to be a map. This does preclude
maps which have purely numeric keys, which should be noted as a
backwards compatibility concern.
|
With this change, is it possible to use a variable of type
|
|
Ah, it's |
|
I'm playing around with lists and maps in 0.7.0-dev (3355c15) and this is the result: main.tf Output I expected to see Version 0.6.16 shows this as a result when only using maps: Is the 0.7.0-dev output expected behavior in 0.7.0? |
This was referenced May 30, 2016
jen20
added a commit
that referenced
this pull request
Jun 11, 2016
The work integrated in #6322 silently broke the ability to use remote state correctly. This commit adds a fix for that, making use of the work integrated in #7124. In order to deal with outputs which are complex structures, we use a forked version of the flatmap package - the difference in the version this commit vs the github.com/hashicorp/terraform/flatmap package is that we add in an additional key for map counts which state requires. Because we bypass the normal helper/schema mechanism, this is not set for us. Because of the HIL type checking of maps, values must be of a homogenous type. This is unfortunate, as it means we can no longer refer to outputs as: ${terraform_remote_state.foo.output.outputname} Instead we had to bring them to the top level namespace: ${terraform_remote_state.foo.outputname} This actually does lead to better overall usability - and the BC breakage is made better by the fact that indexing would have broken the original syntax anyway. We also add a real-world test and assert against specific values. Tests which were previously acceptance tests are now run as unit tests, so regression should be identified at a much earlier stage.
jen20
added a commit
that referenced
this pull request
Jun 11, 2016
The work integrated in #6322 silently broke the ability to use remote state correctly. This commit adds a fix for that, making use of the work integrated in #7124. In order to deal with outputs which are complex structures, we use a forked version of the flatmap package - the difference in the version this commit vs the github.com/hashicorp/terraform/flatmap package is that we add in an additional key for map counts which state requires. Because we bypass the normal helper/schema mechanism, this is not set for us. Because of the HIL type checking of maps, values must be of a homogenous type. This is unfortunate, as it means we can no longer refer to outputs as: ${terraform_remote_state.foo.output.outputname} Instead we had to bring them to the top level namespace: ${terraform_remote_state.foo.outputname} This actually does lead to better overall usability - and the BC breakage is made better by the fact that indexing would have broken the original syntax anyway. We also add a real-world test and assert against specific values. Tests which were previously acceptance tests are now run as unit tests, so regression should be identified at a much earlier stage.
jen20
added a commit
that referenced
this pull request
Jun 11, 2016
The work integrated in #6322 silently broke the ability to use remote state correctly. This commit adds a fix for that, making use of the work integrated in #7124. In order to deal with outputs which are complex structures, we use a forked version of the flatmap package - the difference in the version this commit vs the github.com/hashicorp/terraform/flatmap package is that we add in an additional key for map counts which state requires. Because we bypass the normal helper/schema mechanism, this is not set for us. Because of the HIL type checking of maps, values must be of a homogenous type. This is unfortunate, as it means we can no longer refer to outputs as: ${terraform_remote_state.foo.output.outputname} Instead we had to bring them to the top level namespace: ${terraform_remote_state.foo.outputname} This actually does lead to better overall usability - and the BC breakage is made better by the fact that indexing would have broken the original syntax anyway. We also add a real-world test and assert against specific values. Tests which were previously acceptance tests are now run as unit tests, so regression should be identified at a much earlier stage.
|
can i iterate over list variable like count in tf template |
jen20
added a commit
that referenced
this pull request
Jun 11, 2016
The work integrated in #6322 silently broke the ability to use remote state correctly. This commit adds a fix for that, making use of the work integrated in #7124. In order to deal with outputs which are complex structures, we use a forked version of the flatmap package - the difference in the version this commit vs the github.com/hashicorp/terraform/flatmap package is that we add in an additional key for map counts which state requires. Because we bypass the normal helper/schema mechanism, this is not set for us. Because of the HIL type checking of maps, values must be of a homogenous type. This is unfortunate, as it means we can no longer refer to outputs as: ${terraform_remote_state.foo.output.outputname} Instead we had to bring them to the top level namespace: ${terraform_remote_state.foo.outputname} This actually does lead to better overall usability - and the BC breakage is made better by the fact that indexing would have broken the original syntax anyway. We also add a real-world test and assert against specific values. Tests which were previously acceptance tests are now run as unit tests, so regression should be identified at a much earlier stage.
jen20
added a commit
that referenced
this pull request
Jun 11, 2016
The work integrated in #6322 silently broke the ability to use remote state correctly. This commit adds a fix for that, making use of the work integrated in #7124. In order to deal with outputs which are complex structures, we use a forked version of the flatmap package - the difference in the version this commit vs the github.com/hashicorp/terraform/flatmap package is that we add in an additional key for map counts which state requires. Because we bypass the normal helper/schema mechanism, this is not set for us. Because of the HIL type checking of maps, values must be of a homogenous type. This is unfortunate, as it means we can no longer refer to outputs as: ${terraform_remote_state.foo.output.outputname} Instead we had to bring them to the top level namespace: ${terraform_remote_state.foo.outputname} This actually does lead to better overall usability - and the BC breakage is made better by the fact that indexing would have broken the original syntax anyway. We also add a real-world test and assert against specific values. Tests which were previously acceptance tests are now run as unit tests, so regression should be identified at a much earlier stage.
|
I'm currently trying the new map passing feature of the 0.7-RC2 and I love it. .tfvars file: Error parsing terraform.tfvars: At 32:13: illegal char |
|
Also having some issues with this, am I supposed to be able to declare an output map like this, or am I getting it wrong? I get Version: Terraform v0.7.0-rc2 (46a0709) |
|
@carlpett I have the same issue actually. |
|
Sounds like it's saying the output is a list of maps instead of just one map. |
|
@ocsi01 - does this work in .tfvars? I'm having a similar issue with assigning to an list type var in .tfvars |
|
So I'm probably blind, is there a way to merge maps, or append to a map passed into a module? This is more or less a use case I'm thinking of: |
|
With the mentioned changed in the PR should this be possible? An alternative, working approach is But the former seemed more logical....? |
|
The documentation for |
|
@rjinski currently the block syntax doesn't work as expected due to a quirk of how the configuration language is parsed. The However I totally agree that the former should work, and it hopefully will work that way in a future Terraform version. |
grubernaut
pushed a commit
to hashicorp/terraform-provider-terraform
that referenced
this pull request
Jun 6, 2017
The work integrated in hashicorp/terraform#6322 silently broke the ability to use remote state correctly. This commit adds a fix for that, making use of the work integrated in hashicorp/terraform#7124. In order to deal with outputs which are complex structures, we use a forked version of the flatmap package - the difference in the version this commit vs the github.com/hashicorp/terraform/flatmap package is that we add in an additional key for map counts which state requires. Because we bypass the normal helper/schema mechanism, this is not set for us. Because of the HIL type checking of maps, values must be of a homogenous type. This is unfortunate, as it means we can no longer refer to outputs as: ${terraform_remote_state.foo.output.outputname} Instead we had to bring them to the top level namespace: ${terraform_remote_state.foo.outputname} This actually does lead to better overall usability - and the BC breakage is made better by the fact that indexing would have broken the original syntax anyway. We also add a real-world test and assert against specific values. Tests which were previously acceptance tests are now run as unit tests, so regression should be identified at a much earlier stage.
|
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
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request adds support for native list variables and outputs, building up on the previous change to state. Interpolation functions now return native lists in preference to StringList.
List variables are defined like this:
And outputs ca then be defined in terms of the lists:
This results in the following state:
The result of terraform output is as follows:
Using the output name, an xargs-friendly representation is printed:
The output command also supports indexing into the list (with appropriate range checking and no wrapping):
Along with maps, list outputs from one module may be passed as variables into another, removing the need for the
join(",", var.list_as_string)andsplit(",", var.list_as_string)which was previously necessary in Terraform configuration.This commit also updates the tests and implementations of built-in interpolation functions to take and return native lists where appropriate.
A backwards compatibility note: previously the concat interpolation function was capable of concatenating either strings or lists. The strings use case was deprectated a long time ago but still remained. Because we cannot return
ast.TypeAnyfrom an interpolation function, this use case is no longer supported for strings -concatis onlycapable of concatenating lists. This should not be a huge issue - the type checker picks up incorrect parameters, and the native HIL string concatenation - or the
joinfunction - can be used to replicate the old behaviour.Finally -
StringListis removed, along with all usages of it.Test coverage can still be improved. All existing tests pass, with the exception of some which were testing for configuration which previously was not valid (e.g. lists in variables...). Several out-of-repo test cases used during development can now be converted to context tests and some additional command tests should be written for the
terraform outputcommand.