Closed liamcervante closed 1 year ago
There are quite a few places in the collection conversion code where empty collections are instantiated. Do we also need to strip out optional attributes in any of the other locations?
There are quite a few places in the collection conversion code where empty collections are instantiated. Do we also need to strip out optional attributes in any of the other locations?
I checked this, and I don't think so. Both List
and Set
are done in the main case and Map
doesn't seem to be affected by this particular case (not sure why).
The other times empty lists or sets are instantiated are:
@apparentlymart - could you verify my logic here?
I must admit I'm not sure why this isn't necessary for maps. My understanding of the current state of things is as follows:
cty
currently has cty.Type
overloaded to mean three different overlapping concepts:
null
is substituting for, mainly just to allow the type system to remain coherent when null values are present.cty.Type
is valid only as the second argument to convert.Convert
and the other similar functions in the convert
package.The third case here was a late addition specifically to support the optional attributes concept; prior to this, the convert
package just used type constraints (case 2). Therefore it seems unsurprising to me that there are some leftover spots in the convert
package that don't correctly handle the distinction between case 3 and case 2, and therefore end up using "target type constraints" in situations where normal "type constraints" are expected, such as in the collection element types.
Because optional attributes started off as an experimental thing I didn't want to go infect all of the other codepaths with it, but it seems that Terraform is effectively forcing it to become stable now and so it's time to implement it more robustly. The change here seems like a good step in that direction and I think it's worth merging even though it's probably incomplete.
There are some other things I should add to my todo list to fix up in subsequent changes, building on this:
cty.Type
which explicitly model whether a particular cty.Type
value is a concrete type, a type constrant, or a target type constraint. (I'll probably want to find a better term for "target type constraint" when doing that, because it's confusingly similar to "type constraint").cty.ListValEmpty
, cty.SetValEmpty
, cty.MapValEmpty
, cty.UnknownVal
, and cty.NullVal
(is that all of them?) reject with a panic any cty.Type
value that is a target type constraint, since those are not valid in that context.convert
package also strip optional attributes when the target is a map, even though that is apparently somehow working out okay right now.cty.ObjectWithOptionalAttrs
documentation saying that this mechanism is experimental.In doing the above I may choose to make use of the fact that I marked this as experimental before to make some breaking changes before committing to a final API. However, in doing so I'll aim to make the necessary updates to Terraform/HCL just a matter of search and replace of some different function/method names to try to make the relationships between these things clearer, rather than a significant redesign.
For now though I'm going to merge this as-is because I think it is a good tactical fix to a specific known problem, which need not wait for the more comprehensive changes I described above. Thanks!
Small note for future PRs: I prefer to update the changelog separately from each PR because otherwise PRs tend to get conflicted when I merge them in a different order than they were submitted in. This one worked out okay because I'm merging it immediately, so no harm done here. :grinning:
For https://github.com/hashicorp/terraform/issues/31924
This PR makes the
Convert
function explicitly strip out any nested optional attributes from empty lists and sets as they are converted.I added three unit tests, one each for
list
,set
andmap
. Interestingly,map
conversion works even without the change applied. I wonder if there's something else about the way maps work that means they're not affected by this. I've left the unit test in but haven't made any change to themap
logic.