core: Support explicit variable type declaration

[jen20]

This commit adds support for declaring variable types in Terraform configuration. Historically, the type has been inferred from the default value, defaulting to string if no default was supplied. This has caused users to devise workarounds if they wanted to declare a map but provide values from a .tfvars file (for example).

The new syntax adds the "type" key to variable blocks:

variable "i_am_a_string" {
    type = "string"
}

variable "i_am_a_map" {
    type = "map"
}

This commit does not extend the type system to include bools, integers or floats - the only two types available are maps and strings.

Validation is performed if a default value is provided in order to ensure that the default value type matches the declared type.

In the case that a type is not declared, the old logic is used for determining the type. This allows backwards compatiblity with previous Terraform configuration.

[phinze]

I hear there's one more unit test incoming for that last case, but this code LGTM!

Also could use some docs before merge.

[apparentlymart]

There is an issue filed out there somewhere where the user wanted to use a map of maps of string. Do you think we should prepare for the potential future case of having more complex types in variables by saying something like map<string> instead of just map?

Honestly, part of me is thinking "YAGNI" on this, and certainly later we could decide that map is a backward-compatibility alias for map<string>, if the future of use-cases like that doesn't feel so clear right now. But on the other hand, given that the use-cases for that are already documented in the bug tracker...

[jen20]

@apparentlymart Interesting question. Personally my feeling here is that trying to introduce some form of generic type system is going to be overkill. I guess the use cases for something like that are for e.g. AMIs per region per environment. I am definitely willing to be convinced here though: any thoughts @phinze or @mitchellh?

[phinze]

Nit: tabs/spaces weird here

[phinze]

Nested maps are a good thought to bring up, @apparentlymart, but I think YAGNI / incremental progress is the best strategy for now. We can revisit nested maps after first-class list support is done, which is the current target feature for this chain of implementation work from @jen20. 👍

[phinze]

@jen20 Seems like we might want a test for a sane error message in the case of a type typo (type-o !?):

variable "typeo_no_default" {
  type = "stringg"
}
variable "typeo_with_default" {
  type = "stringg"
  default = "cheese"
}

[phinze]

(On review I believe it should yield the "Variable '%s': must be a string or a map" message, but might as well cover the case in a unit test.)

[jen20]

@phinze Yes that (was) correct. However, I've moved the variable type check into the loader, at the same point as where we validate the default in order to catch errors earlier. Consequently these kinds of errors now get picked up in the config loading tests rather than context validation, which is better in my opinion. I've added a loader test asserting the correct error is received in this case, and squashed all the commits and fixed up some of the spacing weirdness in the docs.

[phinze]

We have the DeclaredType here, can we include it in the error message? I've found that helps immensely with typo discovery.

[phinze]

One last nit, then this LGTM!

[jen20]

Good call @phinze, should make it easier to grep through source. Will merge pending Travis.

[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.