From c3f5177f22b0f43177fc412d8111071f0fffbfec Mon Sep 17 00:00:00 2001 From: Luke Hoban Date: Tue, 13 Jul 2021 07:39:48 -0700 Subject: [PATCH] Fix rendering of diffs for resource without DetailedDiffs (#7500) The diff rendering logic tests whether the DetailedDiff is nil to determine whether to use it for rendering or defer to older older supported approaches to computing diffs. The new logic added in https://github.com/pulumi/pulumi/pull/7226 could lead to replacing a nil DetailedDiff with an empty DetailedDiff, whcih would make the rendering logic believe that a DetailedDiff was present but empty, instead of missing entirely. This change ensures that we maintain nil-ness of the DetailedDiff when we transform it for handling of replaceOnChanges. Also adds tests for this and other cases on applyReplaceOnChanges. Fixes #7486. --- CHANGELOG.md | 3 + pkg/resource/deploy/step_generator.go | 45 ++++++------ pkg/resource/deploy/step_generator_test.go | 81 ++++++++++++++++++++++ 3 files changed, 109 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 035da1e2d..9577ed95a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ CHANGELOG - [sdk/python] - Fix an issue where dependency keys were incorrectly translates to camelcase [#7443](https://github.com/pulumi/pulumi/pull/7443) +- [cli] - Fix rendering of diffs for resource without DetailedDiffs + [#7500](https://github.com/pulumi/pulumi/pull/7500) + ## 3.6.0 (2021-06-30) ### Improvements diff --git a/pkg/resource/deploy/step_generator.go b/pkg/resource/deploy/step_generator.go index 4ab7a3ffd..e8ac73be5 100644 --- a/pkg/resource/deploy/step_generator.go +++ b/pkg/resource/deploy/step_generator.go @@ -527,7 +527,7 @@ func (sg *stepGenerator) generateStepsFromDiff( hasInitErrors := len(old.InitErrors) > 0 // Update the diff to apply any replaceOnChanges annotations and to include initErrors in the diff. - diff, err = sg.applyReplaceOnChanges(diff, goal.ReplaceOnChanges, hasInitErrors) + diff, err = applyReplaceOnChanges(diff, goal.ReplaceOnChanges, hasInitErrors) if err != nil { return nil, result.FromError(err) } @@ -1196,7 +1196,7 @@ const initErrorSpecialKey = "#initerror" // applyReplaceOnChanges adjusts a DiffResult returned from a provider to apply the ReplaceOnChange // settings in the desired state and init errors from the previous state. -func (sg *stepGenerator) applyReplaceOnChanges(diff plugin.DiffResult, +func applyReplaceOnChanges(diff plugin.DiffResult, replaceOnChanges []string, hasInitErrors bool) (plugin.DiffResult, error) { // No further work is necessary for DiffNone unless init errors are present. @@ -1214,23 +1214,26 @@ func (sg *stepGenerator) applyReplaceOnChanges(diff plugin.DiffResult, } // Calculate the new DetailedDiff - modifiedDiff := map[string]plugin.PropertyDiff{} - for p, v := range diff.DetailedDiff { - diffPath, err := resource.ParsePropertyPath(p) - if err != nil { - return diff, err - } - changeToReplace := false - for _, replaceOnChangePath := range replaceOnChangePaths { - if replaceOnChangePath.Contains(diffPath) { - changeToReplace = true - break + var modifiedDiff map[string]plugin.PropertyDiff + if diff.DetailedDiff != nil { + modifiedDiff = map[string]plugin.PropertyDiff{} + for p, v := range diff.DetailedDiff { + diffPath, err := resource.ParsePropertyPath(p) + if err != nil { + return diff, err } + changeToReplace := false + for _, replaceOnChangePath := range replaceOnChangePaths { + if replaceOnChangePath.Contains(diffPath) { + changeToReplace = true + break + } + } + if changeToReplace { + v = v.ToReplace() + } + modifiedDiff[p] = v } - if changeToReplace { - v = v.ToReplace() - } - modifiedDiff[p] = v } // Calculate the new ReplaceKeys @@ -1264,9 +1267,11 @@ func (sg *stepGenerator) applyReplaceOnChanges(diff plugin.DiffResult, } if replaceOnChangePath.Contains(initErrPath) { modifiedReplaceKeys = append(modifiedReplaceKeys, initErrorSpecialKey) - modifiedDiff[initErrorSpecialKey] = plugin.PropertyDiff{ - Kind: plugin.DiffUpdateReplace, - InputDiff: false, + if modifiedDiff != nil { + modifiedDiff[initErrorSpecialKey] = plugin.PropertyDiff{ + Kind: plugin.DiffUpdateReplace, + InputDiff: false, + } } // If an init error is present on a path that causes replacement, then trigger a replacement. modifiedChanges = plugin.DiffSome diff --git a/pkg/resource/deploy/step_generator_test.go b/pkg/resource/deploy/step_generator_test.go index 7e5d62a6f..34e4a8372 100644 --- a/pkg/resource/deploy/step_generator_test.go +++ b/pkg/resource/deploy/step_generator_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" "github.com/stretchr/testify/assert" ) @@ -113,3 +114,83 @@ func TestIgnoreChanges(t *testing.T) { }) } } + +func TestApplyReplaceOnChangesEmptyDetailedDiff(t *testing.T) { + cases := []struct { + name string + diff plugin.DiffResult + replaceOnChanges []string + hasInitErrors bool + expected plugin.DiffResult + }{ + { + name: "Empty diff and replaceOnChanges", + diff: plugin.DiffResult{}, + replaceOnChanges: []string{}, + hasInitErrors: false, + expected: plugin.DiffResult{}, + }, + { + name: "DiffSome and empty replaceOnChanges", + diff: plugin.DiffResult{Changes: plugin.DiffSome, ChangedKeys: []resource.PropertyKey{"a"}}, + replaceOnChanges: []string{}, + hasInitErrors: false, + expected: plugin.DiffResult{Changes: plugin.DiffSome, ChangedKeys: []resource.PropertyKey{"a"}}, + }, + { + name: "DiffSome and non-empty replaceOnChanges", + diff: plugin.DiffResult{Changes: plugin.DiffSome, ChangedKeys: []resource.PropertyKey{"a"}}, + replaceOnChanges: []string{"a"}, + hasInitErrors: false, + expected: plugin.DiffResult{ + Changes: plugin.DiffSome, + ChangedKeys: []resource.PropertyKey{"a"}, + ReplaceKeys: []resource.PropertyKey{"a"}, + }, + }, + { + name: "Empty diff and replaceOnChanges w/ init errors", + diff: plugin.DiffResult{}, + replaceOnChanges: []string{}, + hasInitErrors: true, + expected: plugin.DiffResult{}, + }, + { + name: "DiffSome and empty replaceOnChanges w/ init errors", + diff: plugin.DiffResult{Changes: plugin.DiffSome, ChangedKeys: []resource.PropertyKey{"a"}}, + replaceOnChanges: []string{}, + hasInitErrors: true, + expected: plugin.DiffResult{Changes: plugin.DiffSome, ChangedKeys: []resource.PropertyKey{"a"}}, + }, + { + name: "DiffSome and non-empty replaceOnChanges w/ init errors", + diff: plugin.DiffResult{Changes: plugin.DiffSome, ChangedKeys: []resource.PropertyKey{"a"}}, + replaceOnChanges: []string{"a"}, + hasInitErrors: true, + expected: plugin.DiffResult{ + Changes: plugin.DiffSome, + ChangedKeys: []resource.PropertyKey{"a"}, + ReplaceKeys: []resource.PropertyKey{"a"}, + }, + }, + { + name: "Empty diff and non-empty replaceOnChanges w/ init errors", + diff: plugin.DiffResult{}, + replaceOnChanges: []string{"*"}, + hasInitErrors: true, + expected: plugin.DiffResult{ + Changes: plugin.DiffSome, + ReplaceKeys: []resource.PropertyKey{"#initerror"}, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + newdiff, err := applyReplaceOnChanges(c.diff, c.replaceOnChanges, c.hasInitErrors) + assert.NoError(t, err) + assert.Equal(t, c.expected, newdiff) + }) + } + +}