Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use non-computed property value comparison for matching detailed diff set inputs to plan #2761

Merged
merged 12 commits into from
Dec 23, 2024

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Dec 17, 2024

This PR reworks the way we compute the detailed diff for set type elements in order to handle sets with computed properties better.

Context

There are two reasons it is challenging to produce the detailed diff for set type properties:

  • The planning process re-orders the elements of a set.
  • The engine does not have access to the post-plan properties but can only display diff previews in terms of OldState and NewInputs.

These two reasons means that once we compute the detailed diff in terms of OldState to PlannedState, we need to do a bit of work to transform it into a diff in terms of OldState and NewInputs. Matching PlannedState values to NewInputs values is especially challenging for elements which contain Computed properties as these can change during the planning process. The previous implementation used the hashes of set elements to do the matching but that completely failed for set elements with Computed properties, as the Computed property will change the hash.

New implementation

This implementation instead uses a technique borrowed from opentofu, which is used for a similar purpose. We now do a fuzzy match on the PlannedState value to the NewInputs value, which allows for differences in Computed properties. Non-Computed properties still need to match exactly.

Example

For the schema

"foo": Set{
  Elem: {
     "bar": {Type: string, Optional: true}
     "baz": {Type: string, Computed: true}
  }
}

and the inputs
[{bar: val1}, {bar: val2}]
changing to
[{bar: val1}, {bar: val3}]
we might observe the plan becoming
[{bar: val3, baz: comp3}, {bar: val1, baz: comp1}]

We now need to correctly match the elements to their corresponding inputs, in order to correctly communicate the diff to the engine in terms of OldState and NewInput:

{bar: val3, baz: comp3} should match {bar: val3} (the second element in the input)
and {bar: val1, baz: comp1} should match {bar: val1}

Testing

I've added additional integration tests around Computed in #2740 as well as unit tests here. I'll annotate any left-over issues in the recorded tests to be resolved.

Changes up to 4ba30d9 have the code changes and 4ba30d9 has the test recordings.

Release

Note that this is feature flagged behind Accurate Previews

fixes #2652
fixes #2528

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Dec 17, 2024

This change is part of the following stack:

Change managed by git-spice.

@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from bc6bef7 to 7da1aec Compare December 17, 2024 15:30
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch 2 times, most recently from 8aec9c1 to 1213503 Compare December 17, 2024 19:03
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.69%. Comparing base (f666912) to head (d36172e).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2761      +/-   ##
==========================================
- Coverage   68.72%   68.69%   -0.03%     
==========================================
  Files         322      322              
  Lines       41340    41422      +82     
==========================================
+ Hits        28411    28455      +44     
- Misses      11333    11365      +32     
- Partials     1596     1602       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VenelinMartinov VenelinMartinov marked this pull request as draft December 17, 2024 19:18
@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from 7da1aec to dbcd3ad Compare December 17, 2024 19:22
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch from 1213503 to b9a8bfe Compare December 17, 2024 19:22
@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from dbcd3ad to 12baad1 Compare December 17, 2024 19:34
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch from b9a8bfe to c1e348f Compare December 17, 2024 19:34
@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from 12baad1 to 0d4b52a Compare December 18, 2024 10:07
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch from c1e348f to 5527f63 Compare December 18, 2024 10:07
@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from 0d4b52a to 220bad9 Compare December 18, 2024 10:14
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch from 5527f63 to ed75d48 Compare December 18, 2024 10:14
@VenelinMartinov VenelinMartinov marked this pull request as ready for review December 18, 2024 11:28
@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from 220bad9 to 9ed2faa Compare December 18, 2024 11:46
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch 2 times, most recently from 1199cc2 to 4dc519b Compare December 18, 2024 14:09
@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from 1b16b10 to 689dea2 Compare December 18, 2024 14:13
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch from 935d14a to 2b96b6b Compare December 18, 2024 14:13
@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from 689dea2 to 70fb526 Compare December 18, 2024 14:16
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch from 2b96b6b to 7b8db7c Compare December 18, 2024 14:16
@VenelinMartinov VenelinMartinov force-pushed the vvm/transform_property_value_limit_descent branch from 70fb526 to d32594b Compare December 18, 2024 14:23
@VenelinMartinov VenelinMartinov force-pushed the vvm/two_property_value_walker branch from 11533f0 to d4a1312 Compare December 23, 2024 13:11
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch 3 times, most recently from 2c2666e to cae9e4f Compare December 23, 2024 13:21
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some last comments but this is generally LGTM!

VenelinMartinov added a commit that referenced this pull request Dec 23, 2024
…es simultaneously (#2780)

This PR adds a `walkTwoPropertyValues` property value walker which can
recurse over two property values simultaneously.

This is necessary for checking which input elements match a given plan
element in #2761
Base automatically changed from vvm/two_property_value_walker to master December 23, 2024 15:02
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_sets_v2 branch from bc31087 to d36172e Compare December 23, 2024 15:02
@VenelinMartinov VenelinMartinov enabled auto-merge (squash) December 23, 2024 15:10
@VenelinMartinov VenelinMartinov merged commit daafe0d into master Dec 23, 2024
17 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/detailed_diff_sets_v2 branch December 23, 2024 16:26
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.99.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants