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

0.12.0-rc1 '0.12upgrade' converts lists of maps incorrectly #21281

Closed
Hons opened this issue May 13, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@Hons
Copy link

commented May 13, 2019

I was upgrading a module to work with 0.12 rc1: https://github.com/terraform-aws-modules/terraform-aws-eks
And encountered some upgrade errors.

Terraform Version

$ terraform version
Terraform v0.12.0-rc1
+ provider.aws v2.10.0
+ provider.local v1.2.2
+ provider.null v2.1.2
+ provider.template v2.1.2

Terraform Configuration Files

variable "worker_groups" {
  description = "A list of maps defining worker group configurations to be defined using AWS Launch Configurations. See workers_group_defaults for valid keys."
  type        = "list"

  default = [
    {
      "name" = "default"
    },
  ]
}

Expected Behavior

Either retain the type list, or convert to a list(map).

Actual Behavior

Conversion to a list(string)

variable "worker_groups" {
  description = "A list of maps defining worker group configurations to be defined using AWS Launch Configurations. See workers_group_defaults for valid keys."
  type        = list(string)
  default = [    {
      "name" = "default"
    },
  ]
}

Steps to Reproduce

  1. Put the sample above in a .tf file
  2. Run terraform 0.12upgrade
  3. Check result
@apparentlymart

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Hi @Hons! Thanks for reporting this.

The current behavior is reflective of the fact that lists and maps of anything other than string weren't intentionally supported in prior versions of Terraform and didn't work fully. However, as you saw it did work partially, and thus there are configurations out there relying on the partial support.

A tension we have here is that the previous behavior was resulting from a total lack of type-checking rather than explicit support for lists of maps, and so it isn't possible to specify exactly the same behavior as before within the new type system that now is doing type checking. If we were to rewrite this as list(map(any)), list(map(string)) or list(any) then it would still be imposing some additional constraints compared to Terraform 0.11, because list(any) and map(any) mean "of any single type" -- all of the elements must have the same type -- rather than that each element can have its own type.

The best migration for what it looks like you intended here would be to define this as a list of objects where all of the expected attributes are enumerated:

variable "worker_groups" {
  description = "A list of maps defining worker group configurations to be defined using AWS Launch Configurations. See workers_group_defaults for valid keys."
  type        = list(object({
    name = string
    # ...and any other attributes you expect to see here
  }))
  default = [
    {
      name = "default"
    },
  ]
}

...but the upgrade tool doesn't have enough information to understand that intent; it's currently mentioned only in the human-readable description, so it can't know that an object type is needed and what attributes should be defined in it.

With that said, I think the best approximation of the old behavior we could manage right now is to detect that the default value isn't a list of strings and generate type = any instead, which would then allow freeform values and disable type checking entirely, which is closer to what Terraform v0.11 would've done. That's not ideal because then the caller could potentially pass something that isn't a list at all, but it would at least avoid breaking something that was working in Terraform v0.11.

variable "worker_groups" {
  description = "A list of maps defining worker group configurations to be defined using AWS Launch Configurations. See workers_group_defaults for valid keys."

  # TF-UPGRADE-TODO: Terraform could not infer an exact element type for this list,
  # so it has disabled type checking on this variable altogether. Consider changing this
  # to a more precise type, such as a list of an object type.
  type        = any
  default = [
    {
      name = "default"
    },
  ]
}

If a variable is defined with no default at all, or if the default is an empty list, then we don't have enough information to make this decision at all, and so we'd still generate list(string). I think for that case we'll need to just rely on the upgrade guide to cover why that occurs and what to do about it; this is a situation where the input doesn't have enough machine-readable information available to automatically upgrade it in all cases.

@apparentlymart apparentlymart added this to the v0.12.0 milestone May 13, 2019

@Hons

This comment has been minimized.

Copy link
Author

commented May 14, 2019

Thanks @apparentlymart for the extensive response!
That sounds sensible, some of those upgrade TODOs showed up when i was doing this and were extremely helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.