From 505e6ebbc9514481b3feb994539a6bff5f5d0105 Mon Sep 17 00:00:00 2001 From: Venelin Date: Tue, 17 Dec 2024 11:27:20 +0200 Subject: [PATCH 1/6] disable accurate previews for remainder of pf detailed diff tests --- pkg/pf/tests/diff_test.go | 5 ++++- .../TestDetailedDiffStringAttribute/default/added.golden | 3 +-- .../TestDetailedDiffStringAttribute/default/changed.golden | 1 - .../TestDetailedDiffStringAttribute/default/removed.golden | 1 - .../default_replace/added.golden | 2 +- .../default_replace/changed.golden | 2 +- .../default_replace/removed.golden | 4 ++-- .../TestDetailedDiffStringAttribute/no_replace/added.golden | 1 - .../no_replace/changed.golden | 1 - .../no_replace/removed.golden | 1 - .../plan_modifier_default/added.golden | 3 +-- .../plan_modifier_default/changed.golden | 1 - .../plan_modifier_default_replace/added.golden | 2 +- .../plan_modifier_default_replace/changed.golden | 2 +- .../TestDetailedDiffStringAttribute/replace/added.golden | 2 +- .../TestDetailedDiffStringAttribute/replace/changed.golden | 2 +- .../TestDetailedDiffStringAttribute/replace/removed.golden | 2 +- 17 files changed, 15 insertions(+), 20 deletions(-) diff --git a/pkg/pf/tests/diff_test.go b/pkg/pf/tests/diff_test.go index 3adccf4b7..450b72d1a 100644 --- a/pkg/pf/tests/diff_test.go +++ b/pkg/pf/tests/diff_test.go @@ -195,7 +195,10 @@ func TestDetailedDiffStringAttribute(t *testing.T) { res := pb.NewResource(pb.NewResourceArgs{ ResourceSchema: schema.schema, }) - diff := crosstests.Diff(t, res, map[string]cty.Value{"key": initialValue}, map[string]cty.Value{"key": changeValue}) + diff := crosstests.Diff( + t, res, map[string]cty.Value{"key": initialValue}, map[string]cty.Value{"key": changeValue}, + crosstests.DisableAccurateBridgePreview(), + ) autogold.ExpectFile(t, testOutput{ initialValue: scenario.initialValue, diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/added.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/added.golden index 4029adb59..5f6bb3907 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/added.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/added.golden @@ -22,10 +22,9 @@ Plan: 0 to add, 1 to change, 0 to destroy. ~ testprovider:index/test:Test: (update) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] - ~ key: "default" => "value" + + key: "value" Resources: ~ 1 to update 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/changed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/changed.golden index 39e111cac..023aad9ea 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/changed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/changed.golden @@ -28,5 +28,4 @@ Resources: ~ 1 to update 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/removed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/removed.golden index 80bf0e00e..0b06434ed 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/removed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default/removed.golden @@ -27,5 +27,4 @@ Resources: ~ 1 to update 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/added.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/added.golden index b2d1fe30b..fcfd8dac8 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/added.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/added.golden @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy. +-testprovider:index/test:Test: (replace) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] + ~ id : "test-id" => output ~ key: "default" => "value" Resources: +-1 to replace 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/changed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/changed.golden index ac972eb3a..0249956a6 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/changed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/changed.golden @@ -23,10 +23,10 @@ Plan: 1 to add, 0 to change, 1 to destroy. +-testprovider:index/test:Test: (replace) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] + ~ id : "test-id" => output ~ key: "value" => "value1" Resources: +-1 to replace 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/removed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/removed.golden index 017d9fa09..bccb0d4bb 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/removed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/default_replace/removed.golden @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy. +-testprovider:index/test:Test: (replace) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] - - key: "value" + ~ id : "test-id" => output + ~ key: "value" => "default" Resources: +-1 to replace 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/added.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/added.golden index 5e833a218..0bec01698 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/added.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/added.golden @@ -27,5 +27,4 @@ Resources: ~ 1 to update 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/changed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/changed.golden index 39e111cac..023aad9ea 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/changed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/changed.golden @@ -28,5 +28,4 @@ Resources: ~ 1 to update 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/removed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/removed.golden index 7b7e7290a..0814ae906 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/removed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/no_replace/removed.golden @@ -27,5 +27,4 @@ Resources: ~ 1 to update 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "DELETE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default/added.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default/added.golden index 4029adb59..5f6bb3907 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default/added.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default/added.golden @@ -22,10 +22,9 @@ Plan: 0 to add, 1 to change, 0 to destroy. ~ testprovider:index/test:Test: (update) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] - ~ key: "default" => "value" + + key: "value" Resources: ~ 1 to update 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default/changed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default/changed.golden index 39e111cac..023aad9ea 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default/changed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default/changed.golden @@ -28,5 +28,4 @@ Resources: ~ 1 to update 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default_replace/added.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default_replace/added.golden index b2d1fe30b..fcfd8dac8 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default_replace/added.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default_replace/added.golden @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy. +-testprovider:index/test:Test: (replace) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] + ~ id : "test-id" => output ~ key: "default" => "value" Resources: +-1 to replace 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default_replace/changed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default_replace/changed.golden index ac972eb3a..0249956a6 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default_replace/changed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/plan_modifier_default_replace/changed.golden @@ -23,10 +23,10 @@ Plan: 1 to add, 0 to change, 1 to destroy. +-testprovider:index/test:Test: (replace) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] + ~ id : "test-id" => output ~ key: "value" => "value1" Resources: +-1 to replace 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/added.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/added.golden index b04872625..966e5c53b 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/added.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/added.golden @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy. +-testprovider:index/test:Test: (replace) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] + ~ id : "test-id" => output + key: "value" Resources: +-1 to replace 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/changed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/changed.golden index ac972eb3a..0249956a6 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/changed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/changed.golden @@ -23,10 +23,10 @@ Plan: 1 to add, 0 to change, 1 to destroy. +-testprovider:index/test:Test: (replace) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] + ~ id : "test-id" => output ~ key: "value" => "value1" Resources: +-1 to replace 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "UPDATE"}}, } diff --git a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/removed.golden b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/removed.golden index ac7876c41..33a6ced23 100644 --- a/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/removed.golden +++ b/pkg/pf/tests/testdata/TestDetailedDiffStringAttribute/replace/removed.golden @@ -22,10 +22,10 @@ Plan: 1 to add, 0 to change, 1 to destroy. +-testprovider:index/test:Test: (replace) [id=test-id] [urn=urn:pulumi:test::project::testprovider:index/test:Test::p] + ~ id : "test-id" => output - key: "value" Resources: +-1 to replace 1 unchanged `, - detailedDiff: map[string]interface{}{"key": map[string]interface{}{"kind": "DELETE"}}, } From 23088c90a26ffa6583074cfac6c524740ccef553 Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 16 Dec 2024 12:54:12 +0200 Subject: [PATCH 2/6] Add a fallback for replace decisions to ensure detailed diff is presentation-only --- pkg/pf/tfbridge/provider_diff.go | 7 ++-- pkg/tfbridge/detailed_diff.go | 54 ++++++++++++++++++++++++++++-- pkg/tfbridge/detailed_diff_test.go | 50 ++++++++++++++++++++++++++- pkg/tfbridge/provider.go | 3 +- 4 files changed, 108 insertions(+), 6 deletions(-) diff --git a/pkg/pf/tfbridge/provider_diff.go b/pkg/pf/tfbridge/provider_diff.go index 5341813cb..13c15c976 100644 --- a/pkg/pf/tfbridge/provider_diff.go +++ b/pkg/pf/tfbridge/provider_diff.go @@ -135,7 +135,9 @@ func (p *provider) DiffWithContext( } if providerOpts.enableAccurateBridgePreview { - pluginDetailedDiff, err := calculateDetailedDiff(ctx, &rh, priorState, plannedStateValue, checkedInputs) + replaceOverride := len(replaceKeys) > 0 + pluginDetailedDiff, err := calculateDetailedDiff( + ctx, &rh, priorState, plannedStateValue, checkedInputs, &replaceOverride) if err != nil { return plugin.DiffResult{}, err } @@ -148,7 +150,7 @@ func (p *provider) DiffWithContext( func calculateDetailedDiff( ctx context.Context, rh *resourceHandle, priorState *upgradedResourceState, - plannedStateValue tftypes.Value, checkedInputs resource.PropertyMap, + plannedStateValue tftypes.Value, checkedInputs resource.PropertyMap, replaceOverride *bool, ) (map[string]plugin.PropertyDiff, error) { priorProps, err := convert.DecodePropertyMap(ctx, rh.decoder, priorState.state.Value) if err != nil { @@ -167,6 +169,7 @@ func calculateDetailedDiff( priorProps, props, checkedInputs, + replaceOverride, ) pluginDetailedDiff := make(map[string]plugin.PropertyDiff, len(detailedDiff)) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index e241e90ee..d61467118 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -57,6 +57,21 @@ func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) } } +func containsReplace(m map[string]*pulumirpc.PropertyDiff) bool { + for _, v := range m { + if v.GetKind() == pulumirpc.PropertyDiff_UPDATE_REPLACE { + return true + } + if v.GetKind() == pulumirpc.PropertyDiff_ADD_REPLACE { + return true + } + if v.GetKind() == pulumirpc.PropertyDiff_DELETE_REPLACE { + return true + } + } + return false +} + func promoteToReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff { if diff == nil { return nil @@ -75,6 +90,23 @@ func promoteToReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff { } } +func demoteToNoReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff { + if diff == nil { + return nil + } + kind := diff.GetKind() + switch kind { + case pulumirpc.PropertyDiff_ADD_REPLACE: + return &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_ADD} + case pulumirpc.PropertyDiff_DELETE_REPLACE: + return &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_DELETE} + case pulumirpc.PropertyDiff_UPDATE_REPLACE: + return &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE} + default: + return diff + } +} + type baseDiff string const ( @@ -482,6 +514,7 @@ func MakeDetailedDiffV2( tfs shim.SchemaMap, ps map[string]*SchemaInfo, priorProps, props, newInputs resource.PropertyMap, + replaceOverride *bool, ) map[string]*pulumirpc.PropertyDiff { // Strip secrets and outputs from the properties before calculating the diff. // This allows the rest of the algorithm to focus on the actual changes and not @@ -497,7 +530,23 @@ func MakeDetailedDiffV2( props = stripSecretsAndOutputs(props) newInputs = stripSecretsAndOutputs(newInputs) differ := detailedDiffer{ctx: ctx, tfs: tfs, ps: ps, newInputs: newInputs} - return differ.makeDetailedDiffPropertyMap(priorProps, props) + res := differ.makeDetailedDiffPropertyMap(priorProps, props) + + if replaceOverride != nil { + if containsReplace(res) && !*replaceOverride { + for k, v := range res { + res[k] = demoteToNoReplace(v) + } + } + + if !containsReplace(res) && *replaceOverride { + // We use the internal __meta property to trigger a replace when we have failed to + // determine the correct detailed diff for it. + res["__meta"] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE} + } + } + + return res } func makeDetailedDiffV2( @@ -511,6 +560,7 @@ func makeDetailedDiffV2( assets AssetTable, supportsSecrets bool, newInputs resource.PropertyMap, + replaceOverride *bool, ) (map[string]*pulumirpc.PropertyDiff, error) { // We need to compare the new and olds after all transformations have been applied. // ex. state upgrades, implementation-specific normalizations etc. @@ -532,5 +582,5 @@ func makeDetailedDiffV2( return nil, err } - return MakeDetailedDiffV2(ctx, tfs, ps, priorProps, props, newInputs), nil + return MakeDetailedDiffV2(ctx, tfs, ps, priorProps, props, newInputs, replaceOverride), nil } diff --git a/pkg/tfbridge/detailed_diff_test.go b/pkg/tfbridge/detailed_diff_test.go index f69957ef3..01b6f36a4 100644 --- a/pkg/tfbridge/detailed_diff_test.go +++ b/pkg/tfbridge/detailed_diff_test.go @@ -299,7 +299,7 @@ func runDetailedDiffTest( expected map[string]*pulumirpc.PropertyDiff, ) { t.Helper() - actual := MakeDetailedDiffV2(context.Background(), tfs, ps, old, new, new) + actual := MakeDetailedDiffV2(context.Background(), tfs, ps, old, new, new, nil) require.Equal(t, expected, actual) } @@ -2788,3 +2788,51 @@ func TestDetailedDiffSetHashPanicCaught(t *testing.T) { require.Contains(t, buf.String(), "Failed to calculate preview for element in foo") } + +func TestDetailedDiffReplaceOverrideFalse(t *testing.T) { + t.Parallel() + + old := resource.NewPropertyMapFromMap(map[string]interface{}{ + "foo": "bar", + }) + new := resource.NewPropertyMapFromMap(map[string]interface{}{ + "foo": "baz", + }) + + tfs := shimv2.NewSchemaMap(map[string]*schema.Schema{ + "foo": { + Type: schema.TypeString, + Optional: true, + ForceNew: true, + }, + }) + + actual := MakeDetailedDiffV2(context.Background(), tfs, nil, old, new, new, ref(false)) + require.Equal(t, actual, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) +} + +func TestDetailedDiffReplaceOverrideTrue(t *testing.T) { + t.Parallel() + + old := resource.NewPropertyMapFromMap(map[string]interface{}{ + "foo": "bar", + }) + new := resource.NewPropertyMapFromMap(map[string]interface{}{ + "foo": "baz", + }) + + tfs := shimv2.NewSchemaMap(map[string]*schema.Schema{ + "foo": { + Type: schema.TypeString, + Optional: true, + }, + }) + + actual := MakeDetailedDiffV2(context.Background(), tfs, nil, old, new, new, ref(true)) + require.Equal(t, actual, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + "__meta": {Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE}, + }) +} diff --git a/pkg/tfbridge/provider.go b/pkg/tfbridge/provider.go index 42e8b33ac..15cd9af21 100644 --- a/pkg/tfbridge/provider.go +++ b/pkg/tfbridge/provider.go @@ -1177,8 +1177,9 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum changes = pulumirpc.DiffResponse_DIFF_SOME } + replaceDecision := diff.RequiresNew() detailedDiff, err = makeDetailedDiffV2( - ctx, schema, fields, res.TF, p.tf, state, diff, assets, p.supportsSecrets, news) + ctx, schema, fields, res.TF, p.tf, state, diff, assets, p.supportsSecrets, news, &replaceDecision) if err != nil { return nil, err } From ebe0d437c1c041994fbe2163fec54fde768f09db Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 16 Dec 2024 13:27:14 +0200 Subject: [PATCH 3/6] update test --- pkg/tfbridge/detailed_diff_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/pkg/tfbridge/detailed_diff_test.go b/pkg/tfbridge/detailed_diff_test.go index 01b6f36a4..6e454a320 100644 --- a/pkg/tfbridge/detailed_diff_test.go +++ b/pkg/tfbridge/detailed_diff_test.go @@ -2836,3 +2836,25 @@ func TestDetailedDiffReplaceOverrideTrue(t *testing.T) { "__meta": {Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE}, }) } + +func TestDemoteToNoReplace(t *testing.T) { + t.Parallel() + + diff := &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_ADD_REPLACE} + require.Equal(t, demoteToNoReplace(diff), &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_ADD}) + + diff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_DELETE_REPLACE} + require.Equal(t, demoteToNoReplace(diff), &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_DELETE}) + + diff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE} + require.Equal(t, demoteToNoReplace(diff), &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}) + + diff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_ADD} + require.Equal(t, demoteToNoReplace(diff), &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_ADD}) + + diff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_DELETE} + require.Equal(t, demoteToNoReplace(diff), &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_DELETE}) + + diff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE} + require.Equal(t, demoteToNoReplace(diff), &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}) +} From f3e38a99e819db3bac849489e53bf6620c38eaa7 Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 16 Dec 2024 18:16:57 +0200 Subject: [PATCH 4/6] address reviews --- pkg/tfbridge/detailed_diff.go | 32 +++++++++++++++++------------- pkg/tfbridge/detailed_diff_test.go | 22 ++++++++++++++++++++ 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index d61467118..c6f0d56dc 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -59,13 +59,10 @@ func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) func containsReplace(m map[string]*pulumirpc.PropertyDiff) bool { for _, v := range m { - if v.GetKind() == pulumirpc.PropertyDiff_UPDATE_REPLACE { - return true - } - if v.GetKind() == pulumirpc.PropertyDiff_ADD_REPLACE { - return true - } - if v.GetKind() == pulumirpc.PropertyDiff_DELETE_REPLACE { + switch v.GetKind() { + case pulumirpc.PropertyDiff_UPDATE_REPLACE, + pulumirpc.PropertyDiff_ADD_REPLACE, + pulumirpc.PropertyDiff_DELETE_REPLACE: return true } } @@ -509,6 +506,11 @@ func (differ detailedDiffer) makeDetailedDiffPropertyMap( // MakeDetailedDiffV2 is the main entry point for calculating the detailed diff. // This is an internal function that should not be used outside of the pulumi-terraform-bridge. +// +// The `replaceOverride` parameter is used to override the replace behavior of the detailed diff. +// If true, the diff will be overridden to return a replace. +// If false, the diff will be overridden to not return a replace. +// If nil, the detailed diff will be returned as is. func MakeDetailedDiffV2( ctx context.Context, tfs shim.SchemaMap, @@ -533,17 +535,19 @@ func MakeDetailedDiffV2( res := differ.makeDetailedDiffPropertyMap(priorProps, props) if replaceOverride != nil { - if containsReplace(res) && !*replaceOverride { + if *replaceOverride { + // We need to make sure there is a replace. + if !containsReplace(res) { + // We use the internal __meta property to trigger a replace when we have failed to + // determine the correct detailed diff for it. + res[metaKey] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE} + } + } else { + // There is an override for no replaces, so ensure we don't have any. for k, v := range res { res[k] = demoteToNoReplace(v) } } - - if !containsReplace(res) && *replaceOverride { - // We use the internal __meta property to trigger a replace when we have failed to - // determine the correct detailed diff for it. - res["__meta"] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE} - } } return res diff --git a/pkg/tfbridge/detailed_diff_test.go b/pkg/tfbridge/detailed_diff_test.go index 6e454a320..7a6e59c45 100644 --- a/pkg/tfbridge/detailed_diff_test.go +++ b/pkg/tfbridge/detailed_diff_test.go @@ -2858,3 +2858,25 @@ func TestDemoteToNoReplace(t *testing.T) { diff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE} require.Equal(t, demoteToNoReplace(diff), &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}) } + +func TestContainsReplace(t *testing.T) { + t.Parallel() + + require.True(t, containsReplace(map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE}, + })) + + require.True(t, containsReplace(map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_ADD_REPLACE}, + })) + + require.True(t, containsReplace(map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_DELETE_REPLACE}, + })) + + require.False(t, containsReplace(map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + })) + + require.False(t, containsReplace(map[string]*pulumirpc.PropertyDiff{})) +} From 27e7b547cda4782e877759c1fca7ea5647830ab9 Mon Sep 17 00:00:00 2001 From: Venelin Date: Tue, 17 Dec 2024 11:32:23 +0200 Subject: [PATCH 5/6] Add a fallback for detailed diff replace decisions to ensure detailed diff is presentation-only From 3b77fce91a3ff6b77eb3f20641aa576f272fee96 Mon Sep 17 00:00:00 2001 From: Venelin Date: Tue, 17 Dec 2024 11:55:07 +0200 Subject: [PATCH 6/6] update test --- pkg/tests/detailed_diff_unknown_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/tests/detailed_diff_unknown_test.go b/pkg/tests/detailed_diff_unknown_test.go index b0faa9d9f..05362e63b 100644 --- a/pkg/tests/detailed_diff_unknown_test.go +++ b/pkg/tests/detailed_diff_unknown_test.go @@ -900,7 +900,7 @@ func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) { runTest(t, program2, autogold.Expect(` + prov:index/aux:Aux: (create) [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] - +-prov:index/test:Test: (replace) + ~ prov:index/test:Test: (update) [id=newid] [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] ~ tests: [ @@ -917,7 +917,7 @@ func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) { runTest(t, program2, autogold.Expect(` + prov:index/aux:Aux: (create) [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] - +-prov:index/test:Test: (replace) + ~ prov:index/test:Test: (update) [id=newid] [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] - tests: [ @@ -1026,7 +1026,7 @@ func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) { runTest(t, program2, autogold.Expect(` + prov:index/aux:Aux: (create) [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] - +-prov:index/test:Test: (replace) + ~ prov:index/test:Test: (update) [id=newid] [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] ~ tests: [ @@ -1043,7 +1043,7 @@ func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) { runTest(t, program2, autogold.Expect(` + prov:index/aux:Aux: (create) [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] - +-prov:index/test:Test: (replace) + ~ prov:index/test:Test: (update) [id=newid] [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] - tests: [