helper/schema: track map element counts

[mitchellh]

This adds "field.#" values to the state/diff with the element count of a
map. This fixes a major issue around not knowing when child elements are
computed when doing variable access of a computed map.

Example, if you have a schema like this:

"foo": &Schema{
    Type:     TypeMap,
    Computed: true,
}

And you access it like this in a resource:

${type.name.foo.computed-field}

Then Terraform will error that "field foo could not be found on resource
type.name". By adding that "foo.#" is computed, Terraform core will pick
up that it WILL exist, so its okay.

@svanharmelen Since you've been in helper/schema recently, I'd appreciate a review. Thanks!

[svanharmelen]

@mitchellh I think you missed a bit here. When I add this test:

// #44 - Computed maps
{
  Schema: map[string]*Schema{
    "vars": &Schema{
      Type:     TypeMap,
      Computed: true,
    },
  },

  State: &terraform.InstanceState{
    Attributes: map[string]string{
      "vars.#": "0",
    },
  },

  Config: map[string]interface{}{
    "vars": map[string]interface{}{
      "bar": "${var.foo}",
    },
  },

  ConfigVariables: map[string]string{
    "var.foo": config.UnknownVariableValue,
  },

  Diff: &terraform.InstanceDiff{
    Attributes: map[string]*terraform.ResourceAttrDiff{
      "vars.#": &terraform.ResourceAttrDiff{
        Old:         "0",
        NewComputed: true,
      },
    },
  },

  Err: false,
},

It fails saying NewComputed:false, NewRemoved:true where I would expect NewComputed:true, NewRemoved:false.

What triggered me to check this was PR #661, but I must admit I'm not a 100% sure this test matches the actual use case your trying to fix here...

[mitchellh]

@svanharmelen Good test failure! I'm fixing it now.

[mitchellh]

@svanharmelen Got your test to pass!

[mitchellh]

Merging, I think we're good. :)

[svanharmelen]

Nice!!

[kubek2k]

This change breaks providers that are using maps as elements of their configuration, eg. heroku application or heroku addon. Maybe it would make sense to have a special abstraction layer over those maps, so that they are accessed in one way and persisted the other?

[mitchellh]

@kubek2k Can you describe how it breaks those?

I think long term we do need a way to persist in a different format.

[kubek2k]

for example those lines:
https://github.com/hashicorp/terraform/blob/master/builtin/providers/heroku/resource_heroku_app.go#L353
cause the provider to send down the wire configuration with additional key '#' => '<some_number>'
(this actually doesn't cause the provider to fail)

same is here:
https://github.com/hashicorp/terraform/blob/master/builtin/providers/heroku/resource_heroku_addon.go#L71
causes the provider to send addon configuration with additional key.
So it might be the case that there is more problems like this in the other providers.

BTW: do you have any idea why the author used TypeList of maps instead of map for configuration?

[kubek2k]

@mitchellh zing zing ^^

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