Skip to content

Commit

Permalink
Use non-computed property value comparison for matching detailed diff…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
VenelinMartinov authored Dec 23, 2024
1 parent f666912 commit daafe0d
Show file tree
Hide file tree
Showing 412 changed files with 2,869 additions and 4,300 deletions.
4 changes: 1 addition & 3 deletions pkg/tests/detailed_diff_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -576,14 +576,12 @@ func TestDetailedDiffSet(t *testing.T) {

for _, schemaValueMakerPair := range computedSchemaValueMakerPairs {
t.Run(schemaValueMakerPair.name, func(t *testing.T) {
// TODO[pulumi/pulumi-terraform-bridge#2528]
t.Skipf("These tests are non-deterministic under the non-Accurate Previews bridge.")
t.Parallel()
for _, scenario := range scenarios {
t.Run(scenario.name, func(t *testing.T) {
t.Parallel()
runTest(
t, schemaValueMakerPair.res, schemaValueMakerPair.valueMaker, scenario.initialValue, scenario.changeValue, true,
t, schemaValueMakerPair.res, schemaValueMakerPair.valueMaker, scenario.initialValue, scenario.changeValue, false,
)
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,14 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-crossprovider:index/testRes:TestRes: (replace)
[id=id]
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
+ [0]: {
+ nested : "value"
}
+ tests: [
+ [0]: {
+ nested : "value"
}
]
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "ADD_REPLACE"},
},
detailedDiff: map[string]interface{}{"tests": map[string]interface{}{"kind": "ADD_REPLACE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,8 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
nested : "val1"
}
~ [1]: {
+ __defaults: []
- computed : ""
nested : "val2"
}
+ [2]: {
+ nested : "val3"
Expand All @@ -70,7 +64,8 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[2].nested": map[string]interface{}{"kind": "ADD_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[2]": map[string]interface{}{"kind": "ADD_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,8 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
nested : "val2"
}
~ [1]: {
+ __defaults: []
- computed : ""
nested : "val3"
}
+ [2]: {
+ nested : "val1"
Expand All @@ -70,7 +64,8 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[2].nested": map[string]interface{}{"kind": "ADD_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[2]": map[string]interface{}{"kind": "ADD_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
+ nested : "val1"
+ nested : "val1"
~ nested : "val2" => "val1"
}
~ [1]: {
+ __defaults: []
- computed : ""
~ nested : "val3" => "val2"
~ nested : "val3" => "val2"
}
+ [2]: {
+ nested : "val3"
Expand All @@ -71,7 +66,10 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "ADD_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[2]": map[string]interface{}{"kind": "ADD_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,9 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
~ nested : "val1" => "val2"
~ nested : "val1" => "val2"
}
~ [1]: {
+ __defaults: []
- computed : ""
nested : "val3"
}
+ [2]: {
+ nested : "val1"
Expand All @@ -70,7 +65,9 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "ADD_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[2]": map[string]interface{}{"kind": "ADD_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,9 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
nested : "val1"
}
~ [1]: {
+ __defaults: []
- computed : ""
~ nested : "val3" => "val2"
~ nested : "val3" => "val2"
}
+ [2]: {
+ nested : "val3"
Expand All @@ -70,7 +65,9 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "ADD_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[2]": map[string]interface{}{"kind": "ADD_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
~ nested : "val1" => "val2"
~ nested : "val1" => "val2"
}
~ [1]: {
+ __defaults: []
- computed : ""
~ nested : "val2" => "val3"
~ nested : "val2" => "val3"
}
+ [2]: {
+ nested : "val1"
Expand All @@ -70,7 +66,10 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "ADD_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[2]": map[string]interface{}{"kind": "ADD_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,15 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
~ nested: "value" => "value1"
~ nested : "value" => "value1"
}
]
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{"tests[0].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"}},
detailedDiff: map[string]interface{}{
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,14 @@ Plan: 1 to add, 0 to change, 1 to destroy.
+-crossprovider:index/testRes:TestRes: (replace)
[id=id]
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
+ [0]: {
+ nested : "value"
}
+ tests: [
+ [0]: {
+ nested : "value"
}
]
Resources:
+-1 to replace
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "ADD_REPLACE"},
},
detailedDiff: map[string]interface{}{"tests": map[string]interface{}{"kind": "ADD_REPLACE"}},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,8 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
nested : "val1"
}
~ [1]: {
+ __defaults: []
- computed : ""
nested : "val2"
}
- [2]: {
- computed: ""
Expand All @@ -71,7 +65,8 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[2].nested": map[string]interface{}{"kind": "DELETE_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[2]": map[string]interface{}{"kind": "DELETE_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
~ nested : "val1" => "val2"
~ nested : "val1" => "val2"
}
~ [1]: {
+ __defaults: []
- computed : ""
~ nested : "val2" => "val3"
~ nested : "val2" => "val3"
}
- [2]: {
- computed: ""
Expand All @@ -71,7 +67,10 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "DELETE_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[2]": map[string]interface{}{"kind": "DELETE_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
- nested : "val1"
- nested : "val1"
~ nested : "val1" => "val2"
}
~ [1]: {
+ __defaults: []
- computed : ""
~ nested : "val2" => "val3"
~ nested : "val2" => "val3"
}
- [2]: {
- computed: ""
Expand All @@ -72,7 +67,10 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "DELETE_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[2]": map[string]interface{}{"kind": "DELETE_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,10 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
~ nested : "val1" => "val3"
~ nested : "val1" => "val3"
}
~ [1]: {
+ __defaults: []
- computed : ""
- nested : "val2"
- nested : "val2"
~ nested : "val2" => "val1"
}
- [2]: {
- computed: ""
Expand All @@ -72,7 +67,10 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "DELETE_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[0].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[2]": map[string]interface{}{"kind": "DELETE_REPLACE"},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,9 @@ Plan: 1 to add, 0 to change, 1 to destroy.
[urn=urn:pulumi:test::project::crossprovider:index/testRes:TestRes::example]
~ tests: [
~ [0]: {
+ __defaults: []
- computed : ""
nested : "val1"
}
~ [1]: {
+ __defaults: []
- computed : ""
~ nested : "val2" => "val3"
~ nested : "val2" => "val3"
}
- [2]: {
- computed: ""
Expand All @@ -71,7 +66,9 @@ Resources:
1 unchanged
`,
detailedDiff: map[string]interface{}{
"tests": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "DELETE_REPLACE"},
"tests[0].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].computed": map[string]interface{}{"kind": "UPDATE"},
"tests[1].nested": map[string]interface{}{"kind": "UPDATE_REPLACE"},
"tests[2]": map[string]interface{}{"kind": "DELETE_REPLACE"},
},
}
Loading

0 comments on commit daafe0d

Please sign in to comment.