Tests for collection ForceNew with Unknowns #2515
Merged
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
VenelinMartinov
changed the title
This was referenced Oct 28, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2515 +/- ##
=======================================
Coverage 62.71% 62.71%
=======================================
Files 381 381
Lines 51515 51523 +8
=======================================
+ Hits 32307 32313 +6
- Misses 17395 17398 +3
+ Partials 1813 1812 -1 ☔ View full report in Codecov by Sentry. |
iwahbe
reviewed
Oct 28, 2024
|
Lovely, these tests pass locally. ❤️ |
|
Ah, it's the test matrix - I meant to test without PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW but we test both configurations. I've added an explicit false there. |
This was referenced Oct 28, 2024
t0yv0
reviewed
Oct 28, 2024
| ] | ||
| Resources: | ||
| + 1 to create | ||
| +-1 to replace |
There was a problem hiding this comment.
Possibly a bug. Slightly edge-casy. When something is unknown we do not know that it will of necessity change, but looks like we assume that.
t0yv0
approved these changes
Oct 28, 2024
VenelinMartinov
deleted the
vvm/collection_force_new_detailed_diff_tests
branch
VenelinMartinov
added a commit
that referenced
this pull request
Oct 28, 2024
…ed diff v2 (#2516) This re-records the tests in #2515 under detailed diff v2 to show the changes. The new behaviour is more correct: When the parent of a known property with ForceNew is changed to unknown we now mark the resource for replacement. Both are guesses to the actual effect of the changes but this one seems more likely. stacked on https://github.com/pulumi/pulumi-terraform-bridge/pull/2515\ related to #2496 (comment)
VenelinMartinov
added a commit
that referenced
this pull request
Oct 29, 2024
This PR changes the logic in the detailed diff v2 calculation to short-circuit on encountering nils and unknowns. Previously, when we encountered a null or unknown in either news or olds we would still recurse down and compare the non-nil values versus a nil value in order to get any replacements which might be happening. We would then simplify the diff later to trim these extra diffs but would propagate the replacement to the right level. We would now instead short-circuit on encountering a null or unknown and walk the non-null value to find any properties which can trigger a replacement. This makes it much easier to handle sets in #2451 as recursing into sets is not necessary, as they are only compared by hashes. This change is not meant to change any behaviour and is tested in #2405 Stacked on #2515 and #2516
VenelinMartinov
added a commit
that referenced
this pull request
Oct 29, 2024
VenelinMartinov
added a commit
that referenced
this pull request
Oct 30, 2024
This change adds improved TF set handling to the detailed diff v2. The main challenge here is that pulumi does not have native sets, so set types are represented as lists. ### Diffing sets using the hash ### To correctly find the diff of two sets we calculate the hash of each element ourselves and do the diffs based on that. What makes this somewhat non-trivial is that due to MaxItemsOne flattening we can't just hash the pulumi `PropertyValue`s given to us by the engine. Instead we use `makeSingleTerraformInput` to transform the values using the schema. We then use the hashes of the elements in the set to calculate the diffs. This allows us to correctly account for shuffling and duplicates, matching the terraform behaviour of sets. When returning the element indices back to the engine, we need to return them in terms of oldState and newInputs because the engine does not have access to the plannedState (see #2281). To do that we keep the newInputs and match plannedState elements to their newInputs index by the set hash. Note that this is not possible if the set element contains any computed properties - these can change during the planning process, so we do not attempt to match and print a warning about possibly inaccurate diff results instead. ### Unknowns in sets ### Note that the terraform planning process does not allow a set to contain any unknowns, because that breaks the hashing. Because of that plan should always return an unknown for a set which contains any unknowns. This accounts for cases where resolving the unknown can result in duplicate elements. Unknown elements in sets - the whole set becomes unknown in the plan, so the algorithm no longer works. Currently we return an update for the whole set to the engine and it does the diff on its side. ### Testing ### This PR also includes tests for the set behaviour, both unit tests for the detailed diff algorithm and integration tests using pulumi programs for: - Single element additions, updates and removals - Shuffling, also with additions, updates and removals - Multi-element additions, updates and removals - Unknowns ### Issues ### Builds on #2405 Stacked on #2515, #2516, #2496 and #2497 fixes #2200 fixes #2300 fixes #1904 fixes #186
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 adds tests to record the existing behaviour around ForceNew interacting with collection and collection element changes with unknowns.
Addresses #2496 (comment)