fix flatmap.Expand to work with the schema.Set representation #11042
Conversation
The change in #10787 used flatmap.Expand to fix interpolation of nested maps, but it broke interpolation of sets such that their elements were not represented. For example, the expected string representation of a splatted aws_network_interface.whatever.*.private_ips should be: ``` [{Variable (TypeList): [{Variable (TypeString): 10.41.17.25}]} {Variable (TypeList): [{Variable (TypeString): 10.41.22.236}]}] ``` But instead it became: ``` [{Variable (TypeList): [{Variable (TypeString): }]} {Variable (TypeList): [{Variable (TypeString): }]}] ``` This is because the expandArray function of expand.go treated arrays to exclusively be lists, e.g. not sets. The old code used to match for numeric keys, so it would work for sets, whereas expandArray just assumed keys started at 0 and ascended incrementally. Remember that sets' keys are numeric, but since they are hashes, they can be any integer. The result of assuming that the keys start at 0 led to the recursive call to flatmap.Expand not matching any keys of the set, and returning nil, which is why the above example has nothing where the IP addresses used to be. So we bring back that matching behavior, but we move it to expandArray instead. We've modified it to not reconstruct the data structures like it used to when it was in the Interpolator, and to use the standard int sorter rather than implementing a custom sorter since a custom one is no longer necessary thanks to the use of flatmap.Expand. Fixes #10908, and restores the viability of the workaround I posted in #8696. Big thanks to @jszwedko for helping me with this fix. I was able to diagnose the problem along, but couldn't fix it without his help.
Find the index keys by comparing the strings directly, so we don't need to worry about the prefix value altering the regex.
|
Thanks for this. Your changes on top of mine also make a lot of sense. |
| @@ -42,9 +43,39 @@ func expandArray(m map[string]string, prefix string) []interface{} { | |||
| panic(err) | |||
| } | |||
|
|
|||
| keySet := map[int]bool{} | |||
There was a problem hiding this comment.
Your PR explanation is great, but I'd also add a short comment above here to explain why we're doing this versus the normal incremental range. Changing it would of course fail a test, but in the future I don't want a maintainer to think "but why did jbardin do this?!" Yes, you could always git blame, which is why I said "short comment" to explain the gist and then say something like "see GH-11042 for full details"
|
Thanks @vancluever and sorry we missed your prior work, too. :) |
|
No problem @mitchellh - just happy that it will be fixed! 👍 |
|
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. |
ghost
locked and limited conversation to collaborators
via @2rs2ts
The change in #10787 used flatmap.Expand to fix interpolation of nested
maps, but it broke interpolation of sets such that their elements were
not represented. For example, the expected string representation of a
splatted aws_network_interface.whatever.*.private_ips should be:
But instead it became:
This is because the expandArray function of expand.go treated arrays to
exclusively be lists, e.g. not sets. The old code used to match for
numeric keys, so it would work for sets, whereas expandArray just
assumed keys started at 0 and ascended incrementally. Remember that
sets' keys are numeric, but since they are hashes, they can be any
integer. The result of assuming that the keys start at 0 led to the
recursive call to flatmap.Expand not matching any keys of the set, and
returning nil, which is why the above example has nothing where the IP
addresses used to be.
So we bring back that matching behavior, but we move it to expandArray
instead. We've modified it to not reconstruct the data structures like
it used to when it was in the Interpolator, and to use the standard int
sorter rather than implementing a custom sorter since a custom one is no
longer necessary thanks to the use of flatmap.Expand.
Fixes #10908, and restores the viability of the workaround I posted in #8696.
Big thanks to @jszwedko for helping me with this fix. I was able to
diagnose the problem along, but couldn't fix it without his help.