core: refactoring the way sets work internally v2 #663
Conversation
This is a refactored solution for PR #616. Functionally this is still the same change, but it’s implemented a lot cleaner with less code and less changes to existing parts of TF.
|
@armon @mitchellh please share any comments/feedback on this refactored solution so we can work towards getting this merged. Thx... |
|
Could you please explain at a higher level what this is doing conceptually? I'm having a hard time piecing it together just from the code. |
|
Yeah, I can imagine as I too needed some time to get my head around all the nitty gritty details of what is going on and what depends on what... Very high level this PR changes how sets are handled internally by changing the internal representation from a list to a map. Until now a set was handled as a list (even though the actual implementation was already using a map), so all items were sorted based on their calculated hashes and then received an 'item number' based on a zero-based index. After applying this PR sets are handled as maps where the key is the calculated hash and the value is the the actual item. So by that the 'item number' used to find and/or access an item changed from something that depended on the hashes of the other items in the same set (which determined the order and so the given 'item number'), to a deterministic number related to that specific item. An example of something this fixes could be seen here and here. These (currently commented out) tests fail without this PR. They are now uncommented in this PR as they work as expected (at least how I would expect it 😉) after adding this PR. Next to this primary change I needed to add some additional logic that made sure diffing the diffs (as called here during a So once the variables get interpolated into their actual values, the hash will also change (depending on you given set func of course). To fix this TF will now check if the set item has any values that still need to be interpolated, and if so it will calculate an approximate hash based on the values available at that time. These approximate hashes are shown on screen as something like Does this help clarifying this PR? If not please let me know any specific question you still have. Or if that are too many question or if things still just are not clear enough, then maybe we should do a Skype or Google Hangout to go through the PR? |
| @@ -113,6 +113,25 @@ func (d *ResourceData) HasChange(key string) bool { | |||
| return !reflect.DeepEqual(o, n) | |||
| } | |||
|
|
|||
| // hasComputedSubKeys walks true a schema and returns whether or not the | |||
There was a problem hiding this comment.
typo, not a big deal, but "walks true"
There was a problem hiding this comment.
Whoeps! Will fix as soon as any other comments are in... Thx...
|
This looks great! I'm going to go ahead and merge, I commented on a coupe things, though. |
|
I'm glad you were able to get rid of the modification to |
core: refactoring the way sets work internally v2
|
Nice! Already commented on your reviews and will update them right away so this topic can be closed... |
|
Thanks. :) |
|
Feel free to commit those changes directly as they're all pretty minor. |
- 5.6.17 is no longer a valid mysql engine version, bumping to 5.6.21 - updating security_group_names assertion to match new set structure introduce in hashicorp#663
|
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
This is a refactored solution for PR #616. Functionally this is still the same change, but it’s implemented a lot cleaner with less code and less changes to existing parts of TF.