From 86c603c22ce69cce6504dadbeac219d83201c461 Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 18 Nov 2024 12:57:33 +0000 Subject: [PATCH 1/4] fix provider server detailed diff --- pkg/pf/internal/plugin/provider_server.go | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/pkg/pf/internal/plugin/provider_server.go b/pkg/pf/internal/plugin/provider_server.go index 61b488c05..45273eaa4 100644 --- a/pkg/pf/internal/plugin/provider_server.go +++ b/pkg/pf/internal/plugin/provider_server.go @@ -18,6 +18,8 @@ import ( "context" "encoding/json" "fmt" + "sort" + "strings" "github.com/blang/semver" pbempty "github.com/golang/protobuf/ptypes/empty" @@ -76,13 +78,15 @@ func (p *providerServer) checkNYI(method string, err error) error { return err } -func (p *providerServer) marshalDiff(diff pl.DiffResult) (*pulumirpc.DiffResponse, error) { - changes := pulumirpc.DiffResponse_DIFF_UNKNOWN +func (p *providerServer) marshalDiff(diff plugin.DiffResult) (*pulumirpc.DiffResponse, error) { + var changes pulumirpc.DiffResponse_DiffChanges switch diff.Changes { case pl.DiffNone: changes = pulumirpc.DiffResponse_DIFF_NONE case pl.DiffSome: changes = pulumirpc.DiffResponse_DIFF_SOME + case pl.DiffUnknown: + changes = pulumirpc.DiffResponse_DIFF_UNKNOWN } // Infer the result from the detailed diff. @@ -100,9 +104,16 @@ func (p *providerServer) marshalDiff(diff pl.DiffResult) (*pulumirpc.DiffRespons } else { changes = pulumirpc.DiffResponse_DIFF_SOME + properties := map[string]struct{}{} detailedDiff = make(map[string]*pulumirpc.PropertyDiff) for path, diff := range diff.DetailedDiff { - diffs = append(diffs, path) + for k := range detailedDiff { + // Turn the attribute name into a top-level property name by trimming everything after the first dot. + if firstSep := strings.IndexAny(k, ".["); firstSep != -1 { + k = k[:firstSep] + } + properties[k] = struct{}{} + } var kind pulumirpc.PropertyDiff_Kind switch diff.Kind { @@ -125,6 +136,11 @@ func (p *providerServer) marshalDiff(diff pl.DiffResult) (*pulumirpc.DiffRespons InputDiff: diff.InputDiff, } } + diffs = make([]string, 0, len(properties)) + for k := range properties { + diffs = append(diffs, k) + } + sort.Strings(diffs) } return &pulumirpc.DiffResponse{ @@ -133,6 +149,7 @@ func (p *providerServer) marshalDiff(diff pl.DiffResult) (*pulumirpc.DiffRespons Changes: changes, Diffs: diffs, DetailedDiff: detailedDiff, + HasDetailedDiff: len(detailedDiff) > 0, }, nil } From f035938ee231a0aa4f022de4b0684bcd9fe6eca6 Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 18 Nov 2024 13:33:11 +0000 Subject: [PATCH 2/4] pass through diffs and replaces --- pkg/pf/internal/plugin/provider_server.go | 56 +++++------------------ 1 file changed, 11 insertions(+), 45 deletions(-) diff --git a/pkg/pf/internal/plugin/provider_server.go b/pkg/pf/internal/plugin/provider_server.go index 45273eaa4..17d8df2ca 100644 --- a/pkg/pf/internal/plugin/provider_server.go +++ b/pkg/pf/internal/plugin/provider_server.go @@ -18,8 +18,6 @@ import ( "context" "encoding/json" "fmt" - "sort" - "strings" "github.com/blang/semver" pbempty "github.com/golang/protobuf/ptypes/empty" @@ -92,55 +90,23 @@ func (p *providerServer) marshalDiff(diff plugin.DiffResult) (*pulumirpc.DiffRes // Infer the result from the detailed diff. var diffs, replaces []string var detailedDiff map[string]*pulumirpc.PropertyDiff - if len(diff.DetailedDiff) == 0 { - diffs = make([]string, len(diff.ChangedKeys)) - for i, k := range diff.ChangedKeys { - diffs[i] = string(k) - } - replaces = make([]string, len(diff.ReplaceKeys)) - for i, k := range diff.ReplaceKeys { - replaces[i] = string(k) - } - } else { - changes = pulumirpc.DiffResponse_DIFF_SOME - - properties := map[string]struct{}{} + if len(diff.DetailedDiff) != 0 { detailedDiff = make(map[string]*pulumirpc.PropertyDiff) for path, diff := range diff.DetailedDiff { - for k := range detailedDiff { - // Turn the attribute name into a top-level property name by trimming everything after the first dot. - if firstSep := strings.IndexAny(k, ".["); firstSep != -1 { - k = k[:firstSep] - } - properties[k] = struct{}{} - } - - var kind pulumirpc.PropertyDiff_Kind - switch diff.Kind { - case pl.DiffAdd: - kind = pulumirpc.PropertyDiff_ADD - case pl.DiffAddReplace: - kind, replaces = pulumirpc.PropertyDiff_ADD_REPLACE, append(replaces, path) - case pl.DiffDelete: - kind = pulumirpc.PropertyDiff_DELETE - case pl.DiffDeleteReplace: - kind, replaces = pulumirpc.PropertyDiff_DELETE, append(replaces, path) - case pl.DiffUpdate: - kind = pulumirpc.PropertyDiff_UPDATE - case pl.DiffUpdateReplace: - kind, replaces = pulumirpc.PropertyDiff_UPDATE_REPLACE, append(replaces, path) - } - detailedDiff[path] = &pulumirpc.PropertyDiff{ - Kind: kind, + Kind: pulumirpc.PropertyDiff_Kind(diff.Kind), //nolint:gosec InputDiff: diff.InputDiff, } } - diffs = make([]string, 0, len(properties)) - for k := range properties { - diffs = append(diffs, k) - } - sort.Strings(diffs) + } + + diffs = make([]string, len(diff.ChangedKeys)) + for i, k := range diff.ChangedKeys { + diffs[i] = string(k) + } + replaces = make([]string, len(diff.ReplaceKeys)) + for i, k := range diff.ReplaceKeys { + replaces[i] = string(k) } return &pulumirpc.DiffResponse{ From 10b8e7510e9b53751852a491a83a556b02d9a7a4 Mon Sep 17 00:00:00 2001 From: Venelin Date: Fri, 22 Nov 2024 17:41:24 +0000 Subject: [PATCH 3/4] unit tests for marshal diff --- .../internal/plugin/provider_server_test.go | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 pkg/pf/internal/plugin/provider_server_test.go diff --git a/pkg/pf/internal/plugin/provider_server_test.go b/pkg/pf/internal/plugin/provider_server_test.go new file mode 100644 index 000000000..c08070f42 --- /dev/null +++ b/pkg/pf/internal/plugin/provider_server_test.go @@ -0,0 +1,103 @@ +package plugin + +import ( + "testing" + + "github.com/hexops/autogold/v2" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" + pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go" + "github.com/stretchr/testify/require" +) + +// autogold gets confused about the enums, so we just re-define them here. +// +//nolint:revive +const ( + ADD pulumirpc.PropertyDiff_Kind = 0 + ADD_REPLACE pulumirpc.PropertyDiff_Kind = 1 + DELETE pulumirpc.PropertyDiff_Kind = 2 + DELETE_REPLACE pulumirpc.PropertyDiff_Kind = 3 + UPDATE pulumirpc.PropertyDiff_Kind = 4 + UPDATE_REPLACE pulumirpc.PropertyDiff_Kind = 5 +) + +//nolint:revive +const ( + DIFF_UNKNOWN pulumirpc.DiffResponse_DiffChanges = 0 + DIFF_NONE pulumirpc.DiffResponse_DiffChanges = 1 + DIFF_SOME pulumirpc.DiffResponse_DiffChanges = 2 +) + +//nolint:unconvert +func TestMarshalDiff(t *testing.T) { + t.Parallel() + + runTest := func(t *testing.T, diff plugin.DiffResult) *pulumirpc.DiffResponse { + server := providerServer{} + resp, err := server.marshalDiff(diff) + require.NoError(t, err) + return resp + } + + t.Run("no diffs", func(t *testing.T) { + diff := plugin.DiffResult{ + Changes: plugin.DiffNone, + ReplaceKeys: []resource.PropertyKey{}, + ChangedKeys: []resource.PropertyKey{}, + DetailedDiff: map[string]plugin.PropertyDiff{}, + } + + autogold.Expect(&pulumirpc.DiffResponse{ + Replaces: []string{}, + Changes: pulumirpc.DiffResponse_DIFF_NONE, + Diffs: []string{}, + }).Equal(t, runTest(t, diff)) + }) + + t.Run("diff without detailed diff", func(t *testing.T) { + diff := plugin.DiffResult{ + Changes: plugin.DiffSome, + ReplaceKeys: []resource.PropertyKey{"replace"}, + ChangedKeys: []resource.PropertyKey{"change"}, + } + + autogold.Expect(&pulumirpc.DiffResponse{ + Replaces: []string{"replace"}, + Changes: pulumirpc.DiffResponse_DIFF_SOME, + Diffs: []string{"change"}, + }).Equal(t, runTest(t, diff)) + }) + + t.Run("diff with detailed diff", func(t *testing.T) { + diff := plugin.DiffResult{ + Changes: plugin.DiffSome, + ReplaceKeys: []resource.PropertyKey{"replace"}, + ChangedKeys: []resource.PropertyKey{"change", "replace"}, + DetailedDiff: map[string]plugin.PropertyDiff{ + "change": { + Kind: plugin.DiffAdd, + }, + "replace": { + Kind: plugin.DiffDeleteReplace, + }, + }, + } + + autogold.Expect(&pulumirpc.DiffResponse{ + Replaces: []string{ + "replace", + }, + Changes: pulumirpc.DiffResponse_DiffChanges(DIFF_SOME), + Diffs: []string{ + "change", + "replace", + }, + DetailedDiff: map[string]*pulumirpc.PropertyDiff{ + "change": {}, + "replace": {Kind: pulumirpc.PropertyDiff_Kind(DELETE_REPLACE)}, + }, + HasDetailedDiff: true, + }).Equal(t, runTest(t, diff)) + }) +} From 12bb0f7510135c0869d042b418b03c8ebe036cd9 Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 25 Nov 2024 12:32:58 +0000 Subject: [PATCH 4/4] address review --- pkg/pf/internal/plugin/provider_server.go | 38 ++++++++++++++----- .../internal/plugin/provider_server_test.go | 37 ++++-------------- 2 files changed, 37 insertions(+), 38 deletions(-) diff --git a/pkg/pf/internal/plugin/provider_server.go b/pkg/pf/internal/plugin/provider_server.go index 17d8df2ca..4254c3309 100644 --- a/pkg/pf/internal/plugin/provider_server.go +++ b/pkg/pf/internal/plugin/provider_server.go @@ -23,9 +23,9 @@ import ( pbempty "github.com/golang/protobuf/ptypes/empty" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" "github.com/pulumi/pulumi/sdk/v3/go/common/resource/config" - "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" pl "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" "github.com/pulumi/pulumi/sdk/v3/go/common/tokens" + "github.com/pulumi/pulumi/sdk/v3/go/common/util/contract" pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -76,7 +76,27 @@ func (p *providerServer) checkNYI(method string, err error) error { return err } -func (p *providerServer) marshalDiff(diff plugin.DiffResult) (*pulumirpc.DiffResponse, error) { +func pluginDiffKindToRPC(kind pl.DiffKind) pulumirpc.PropertyDiff_Kind { + switch kind { + case pl.DiffAdd: + return pulumirpc.PropertyDiff_ADD + case pl.DiffAddReplace: + return pulumirpc.PropertyDiff_ADD_REPLACE + case pl.DiffDelete: + return pulumirpc.PropertyDiff_DELETE + case pl.DiffDeleteReplace: + return pulumirpc.PropertyDiff_DELETE_REPLACE + case pl.DiffUpdate: + return pulumirpc.PropertyDiff_UPDATE + case pl.DiffUpdateReplace: + return pulumirpc.PropertyDiff_UPDATE_REPLACE + default: + contract.Assertf(false, "unknown diff kind: %v", kind) + return pulumirpc.PropertyDiff_ADD + } +} + +func (p *providerServer) marshalDiff(diff pl.DiffResult) (*pulumirpc.DiffResponse, error) { var changes pulumirpc.DiffResponse_DiffChanges switch diff.Changes { case pl.DiffNone: @@ -94,7 +114,7 @@ func (p *providerServer) marshalDiff(diff plugin.DiffResult) (*pulumirpc.DiffRes detailedDiff = make(map[string]*pulumirpc.PropertyDiff) for path, diff := range diff.DetailedDiff { detailedDiff[path] = &pulumirpc.PropertyDiff{ - Kind: pulumirpc.PropertyDiff_Kind(diff.Kind), //nolint:gosec + Kind: pluginDiffKindToRPC(diff.Kind), InputDiff: diff.InputDiff, } } @@ -120,21 +140,21 @@ func (p *providerServer) marshalDiff(diff plugin.DiffResult) (*pulumirpc.DiffRes } type forwardServer struct { - plugin.UnimplementedProvider + pl.UnimplementedProvider - parameterize func(context.Context, plugin.ParameterizeRequest) (plugin.ParameterizeResponse, error) + parameterize func(context.Context, pl.ParameterizeRequest) (pl.ParameterizeResponse, error) } func (p forwardServer) Parameterize( - ctx context.Context, req plugin.ParameterizeRequest, -) (plugin.ParameterizeResponse, error) { + ctx context.Context, req pl.ParameterizeRequest, +) (pl.ParameterizeResponse, error) { return p.parameterize(ctx, req) } func (p *providerServer) Parameterize( ctx context.Context, req *pulumirpc.ParameterizeRequest, ) (*pulumirpc.ParameterizeResponse, error) { - return plugin.NewProviderServer(&forwardServer{ + return pl.NewProviderServer(&forwardServer{ parameterize: p.provider.ParameterizeWithContext, }).Parameterize(ctx, req) } @@ -150,7 +170,7 @@ func (p *providerServer) GetSchema(ctx context.Context, } subpackageVersion = &ver } - schema, err := p.provider.GetSchemaWithContext(ctx, plugin.GetSchemaRequest{ + schema, err := p.provider.GetSchemaWithContext(ctx, pl.GetSchemaRequest{ Version: req.GetVersion(), SubpackageName: req.SubpackageName, SubpackageVersion: subpackageVersion, diff --git a/pkg/pf/internal/plugin/provider_server_test.go b/pkg/pf/internal/plugin/provider_server_test.go index c08070f42..dcac92f09 100644 --- a/pkg/pf/internal/plugin/provider_server_test.go +++ b/pkg/pf/internal/plugin/provider_server_test.go @@ -3,33 +3,12 @@ package plugin import ( "testing" - "github.com/hexops/autogold/v2" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go" "github.com/stretchr/testify/require" ) -// autogold gets confused about the enums, so we just re-define them here. -// -//nolint:revive -const ( - ADD pulumirpc.PropertyDiff_Kind = 0 - ADD_REPLACE pulumirpc.PropertyDiff_Kind = 1 - DELETE pulumirpc.PropertyDiff_Kind = 2 - DELETE_REPLACE pulumirpc.PropertyDiff_Kind = 3 - UPDATE pulumirpc.PropertyDiff_Kind = 4 - UPDATE_REPLACE pulumirpc.PropertyDiff_Kind = 5 -) - -//nolint:revive -const ( - DIFF_UNKNOWN pulumirpc.DiffResponse_DiffChanges = 0 - DIFF_NONE pulumirpc.DiffResponse_DiffChanges = 1 - DIFF_SOME pulumirpc.DiffResponse_DiffChanges = 2 -) - -//nolint:unconvert func TestMarshalDiff(t *testing.T) { t.Parallel() @@ -48,11 +27,11 @@ func TestMarshalDiff(t *testing.T) { DetailedDiff: map[string]plugin.PropertyDiff{}, } - autogold.Expect(&pulumirpc.DiffResponse{ + require.Equal(t, &pulumirpc.DiffResponse{ Replaces: []string{}, Changes: pulumirpc.DiffResponse_DIFF_NONE, Diffs: []string{}, - }).Equal(t, runTest(t, diff)) + }, runTest(t, diff)) }) t.Run("diff without detailed diff", func(t *testing.T) { @@ -62,11 +41,11 @@ func TestMarshalDiff(t *testing.T) { ChangedKeys: []resource.PropertyKey{"change"}, } - autogold.Expect(&pulumirpc.DiffResponse{ + require.Equal(t, &pulumirpc.DiffResponse{ Replaces: []string{"replace"}, Changes: pulumirpc.DiffResponse_DIFF_SOME, Diffs: []string{"change"}, - }).Equal(t, runTest(t, diff)) + }, runTest(t, diff)) }) t.Run("diff with detailed diff", func(t *testing.T) { @@ -84,20 +63,20 @@ func TestMarshalDiff(t *testing.T) { }, } - autogold.Expect(&pulumirpc.DiffResponse{ + require.Equal(t, &pulumirpc.DiffResponse{ Replaces: []string{ "replace", }, - Changes: pulumirpc.DiffResponse_DiffChanges(DIFF_SOME), + Changes: pulumirpc.DiffResponse_DIFF_SOME, Diffs: []string{ "change", "replace", }, DetailedDiff: map[string]*pulumirpc.PropertyDiff{ "change": {}, - "replace": {Kind: pulumirpc.PropertyDiff_Kind(DELETE_REPLACE)}, + "replace": {Kind: pulumirpc.PropertyDiff_DELETE_REPLACE}, }, HasDetailedDiff: true, - }).Equal(t, runTest(t, diff)) + }, runTest(t, diff)) }) }