List parameters should be flattened if MaxItems == 1

[mmdriley]

Terraform's Schema doesn't have an explicit way to indicate "a named group of parameters". To approximate this, providers use a schema.TypeList of schema.Resource with MaxItems: 1. This is pretty common in the AWS provider.

As a concrete example, the aws_alb_target_group resource (doc, src) has an optional group named health_check with MaxItems set to 1. This matches reality, since a target group has exactly one health check.

The bridge brings this field in as an array -- originally as healthCheck, though we explicitly pluralized it so its name matched its type. In this case, though, the type should match the name -- this parameter should be an optional object.

A quick spot check through other instances where MaxItems == 1 suggests this is true generally. We should be projecting lists of length <= 1 as maybe-optional objects.

[lukehoban]

Interesting - I had noticed a couple other examples of this and thought is was just odd design decision in the Terraform providers, but given the way these get exposed to users in Terraform, it actually makes sense to use lists with max one item as a way of nesting properties.

As you note though - projecting this as an array make far less sense in Pulumi.

[pgavlin]

@joeduffy @lukehoban are we comfortable moving this to 0.11?

[joeduffy]

I guess ... this is a breaking change, though, right?

[pgavlin]

Yep, and that's why I ask.

[lukehoban]

I've generally been moving big breaking changes in libraries to 0.11, with the assumption that https://github.com/pulumi/pulumi-service/issues/15 will be addressed in 0.10 giving us a way to make breaking changes in our libraries without forcing users to take those breaking changes immediately. This will provide a much better answer for how we deploy breaking changes to customers.

[lukehoban]

Note - this is very visible for Kubernetes - see https://github.com/pulumi/pulumi-kubernetes/pull/2/files#diff-2f88940fd609897ac656a04d3cb7c464.

 let nginxvolume = new kubernetes.PersistentVolume("redis", {
         metadata: [{
                 name: "nginxvolume"
         }],
         spec: [{
                 capacity: {
                         storage: "10Gi"
                 },
                 accessModes: ["ReadWriteMany"],
                 persistentVolumeSource: [{
                         gcePersistentDisk: [{
                                 pdName: "test-123"
                         }]
                 }]
         }]
 });

[pgavlin]

I assume we still believe that this is important to address in 0.11, especially given the impact on k8s?

[lukehoban]

Yes - would love to get this done sooner rather than later - it will be a meaningful breaking change.

One thing I noted to @joeduffy in person yesterday - if we make this change to projection then some things which are not breaking changes for Terraform (increasing MaxItems from 1 to 2), will be breaking changes for us (changing type from scalar to array).

I'm not sure if there is any good alternative to that. And I think generally we'll have to treat adopting new versions of the Terraform provider as (likely) breaking changes (version bumps) in our provider.

But if anyone has ideas on how we could enable this sort of thing to not be a breaking change - that would be even better.

[pgavlin]

But if anyone has ideas on how we could enable this sort of thing to not be a breaking change - that would be even better.

We could always type these as foo | foo[], right?

[lukehoban]

We could always type these as foo | foo[], right?

Would we do this on the input side for all arrays?

If we only do it for MaxItems=1 then it's still a breaking change when TF changes it to 2.

I assume we would on the output side always make it an array - even in the MaxItems=1 case.

There's a little bit of subtlety there, but I could see that working okay in practice.

[pgavlin]

If we only do it for MaxItems=1 then it's still a breaking change when TF changes it to 2.

Yes, that's a fair point. Would it be too odd to do this for all arrays?