Add SDKv2 Detailed Diff tests for Computed properties in sets #2740
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is part of the following stack: Change managed by git-spice. |
This was referenced Dec 12, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2740 +/- ##
=======================================
Coverage 68.60% 68.60%
=======================================
Files 322 322
Lines 41223 41223
=======================================
Hits 28282 28282
Misses 11348 11348
Partials 1593 1593 ☔ View full report in Codecov by Sentry. |
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_lists_many_elements
branch
from
7c0b26d to
d7e5748
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
7c5f7c4 to
d8b1526
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_lists_many_elements
branch
from
d7e5748 to
9ce613b
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
d8b1526 to
bd80587
Compare
Base automatically changed from
vvm/sdkv2_detailed_diff_lists_many_elements
to
master
December 13, 2024 19:17
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
bd80587 to
8afffe0
Compare
VenelinMartinov
changed the base branch from
master
to
vvm/return_detailed_diff_only_if_diff_some
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
8afffe0 to
8f2b3d6
Compare
This was referenced Dec 16, 2024
VenelinMartinov
force-pushed
the
vvm/return_detailed_diff_only_if_diff_some
branch
from
995278d to
edbbf6c
Compare
VenelinMartinov
changed the base branch from
vvm/return_detailed_diff_only_if_diff_some
to
vvm/sdkv2_detailed_diff_replace_tests
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
8f2b3d6 to
23ca148
Compare
|
Mark it ready for review when ready 🙏 |
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_replace_tests
branch
from
cede42b to
ee3ff32
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
23ca148 to
983e8df
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_replace_tests
branch
from
ee3ff32 to
6c8be27
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_replace_tests
branch
from
fc613aa to
f64719e
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
9d2b6a5 to
1310b4d
Compare
This was referenced Dec 17, 2024
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_replace_tests
branch
from
f64719e to
9c9e1d3
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
1310b4d to
3504da5
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_replace_tests
branch
from
9c9e1d3 to
4ea302c
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
3504da5 to
c99f64d
Compare
Base automatically changed from
vvm/sdkv2_detailed_diff_replace_tests
to
master
December 17, 2024 14:34
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
c99f64d to
4c3deba
Compare
This was referenced Dec 17, 2024
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
fed65d6 to
8e6731f
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
2 times, most recently
from
6e6a723 to
fff242e
Compare
VenelinMartinov
force-pushed
the
vvm/sdkv2_detailed_diff_computed_in_sets
branch
from
fff242e to
eae4813
Compare
iwahbe
approved these changes
Dec 18, 2024
VenelinMartinov
added a commit
that referenced
this pull request
Dec 23, 2024
… set inputs to plan (#2761) 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](https://github.com/opentofu/opentofu/blob/cb2e9119aa75eeb8e1fa175e2b7a205a4fef129f/internal/plans/objchange/objchange.go#L433) 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](4ba30d9) have the code changes and [4ba30d9](4ba30d9) has the test recordings. ## Release Note that this is feature flagged behind Accurate Previews fixes #2652 fixes #2528
|
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds tests around Computed properties in Sets for SDKv2, similar to what we have on the PF side.
set attributes:
For the block tests each test has a variant with the computed property unspecified in the program and one where the property is specified in the user program.
set blocks:
Note that all these tests are currently run without Accurate Previews, because of #2528. I will enable the Accurate Previews feature flag for these tests along with the fix for 2528, so that we can evaluate the differences compared to not running the feature flag.
The output is non-deterministic without Accurate Previews, so the tests are recorded but skipped.
necessary for #2528