From 108d366a04647b4ed6d3368c3aa8f7c1ef6979b2 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 26 Jan 2024 00:18:19 +0300 Subject: [PATCH] Rename conversion.TerraformedConversion to conversion.ManagedConversion - Add unit tests for conversion.RoundTrip Signed-off-by: Alper Rifat Ulucinar --- pkg/config/conversion/conversions.go | 27 ++--- pkg/config/conversion/conversions_test.go | 3 +- pkg/controller/conversion/functions.go | 19 ++-- pkg/controller/conversion/functions_test.go | 103 ++++++++++++++++++++ pkg/controller/conversion/registry.go | 36 ++++--- pkg/resource/fake/terraformed.go | 30 ++++++ 6 files changed, 186 insertions(+), 32 deletions(-) create mode 100644 pkg/controller/conversion/functions_test.go diff --git a/pkg/config/conversion/conversions.go b/pkg/config/conversion/conversions.go index 0b327ffd..ba5a2da1 100644 --- a/pkg/config/conversion/conversions.go +++ b/pkg/config/conversion/conversions.go @@ -49,18 +49,18 @@ type PavedConversion interface { ConvertPaved(src, target *fieldpath.Paved) (bool, error) } -// TerraformedConversion defines a Conversion from a specific source -// resource.Terraformed type to a target one. Generic Conversion +// ManagedConversion defines a Conversion from a specific source +// resource.Managed type to a target one. Generic Conversion // implementations may prefer to implement the PavedConversion interface. -// Implementations of TerraformedConversion can do type assertions to -// specific source and target types and so they are expected to be +// Implementations of ManagedConversion can do type assertions to +// specific source and target types, and so, they are expected to be // strongly typed. -type TerraformedConversion interface { +type ManagedConversion interface { Conversion - // ConvertTerraformed converts from the `src` managed resource to the `dst` + // ConvertManaged converts from the `src` managed resource to the `dst` // managed resource and returns `true` if the conversion has been done, // `false` otherwise, together with any errors encountered. - ConvertTerraformed(src, target resource.Managed) (bool, error) + ConvertManaged(src, target resource.Managed) (bool, error) } type baseConversion struct { @@ -124,16 +124,17 @@ type customConversion struct { customConverter customConverter } -func (cc *customConversion) ConvertTerraformed(src, target resource.Managed) (bool, error) { - if !cc.Applicable(src, target) { +func (cc *customConversion) ConvertManaged(src, target resource.Managed) (bool, error) { + if !cc.Applicable(src, target) || cc.customConverter == nil { return false, nil } - if err := cc.customConverter(src, target); err != nil { - return false, err - } - return true, nil + return true, errors.Wrap(cc.customConverter(src, target), "failed to apply the converter function") } +// NewCustomConverter returns a new Conversion from the specified +// `sourceVersion` of an API to the specified `targetVersion` and invokes +// the specified converter function to perform the conversion on the +// managed resources. func NewCustomConverter(sourceVersion, targetVersion string, converter func(src, target resource.Managed) error) Conversion { return &customConversion{ baseConversion: newBaseConversion(sourceVersion, targetVersion), diff --git a/pkg/config/conversion/conversions_test.go b/pkg/config/conversion/conversions_test.go index 33f09a70..03de9080 100644 --- a/pkg/config/conversion/conversions_test.go +++ b/pkg/config/conversion/conversions_test.go @@ -8,11 +8,10 @@ import ( "fmt" "testing" + "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/google/go-cmp/cmp" "k8s.io/utils/ptr" - - "github.com/crossplane/crossplane-runtime/pkg/fieldpath" ) const ( diff --git a/pkg/controller/conversion/functions.go b/pkg/controller/conversion/functions.go index 0b6c30a9..33619094 100644 --- a/pkg/controller/conversion/functions.go +++ b/pkg/controller/conversion/functions.go @@ -15,8 +15,8 @@ import ( // RoundTrip round-trips from `src` to `dst` via an unstructured map[string]any // representation of the `src` object and applies the registered webhook -// conversion functions. -func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // considered breaking this according to the converters and I did not like it +// conversion functions of this registry. +func (r *registry) RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // considered breaking this according to the converters and I did not like it srcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(src) if err != nil { return errors.Wrap(err, "cannot convert the conversion source object into the map[string]any representation") @@ -35,7 +35,7 @@ func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // consid } srcPaved := fieldpath.Pave(srcMap) dstPaved := fieldpath.Pave(dstMap) - for _, c := range GetConversions(dst) { + for _, c := range r.GetConversions(dst) { if pc, ok := c.(conversion.PavedConversion); ok { if _, err := pc.ConvertPaved(srcPaved, dstPaved); err != nil { return errors.Wrapf(err, "cannot apply the PavedConversion for the %q object", dst.GetTerraformResourceType()) @@ -48,9 +48,9 @@ func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // consid return errors.Wrap(err, "cannot convert the map[string]any representation of the conversion target object to the target object") } - for _, c := range GetConversions(dst) { - if tc, ok := c.(conversion.TerraformedConversion); ok { - if _, err := tc.ConvertTerraformed(src, dst); err != nil { + for _, c := range r.GetConversions(dst) { + if tc, ok := c.(conversion.ManagedConversion); ok { + if _, err := tc.ConvertManaged(src, dst); err != nil { return errors.Wrapf(err, "cannot apply the TerraformedConversion for the %q object", dst.GetTerraformResourceType()) } } @@ -58,3 +58,10 @@ func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // consid return nil } + +// RoundTrip round-trips from `src` to `dst` via an unstructured map[string]any +// representation of the `src` object and applies the registered webhook +// conversion functions. +func RoundTrip(dst, src resource.Terraformed) error { + return instance.RoundTrip(dst, src) +} diff --git a/pkg/controller/conversion/functions_test.go b/pkg/controller/conversion/functions_test.go new file mode 100644 index 00000000..1e7ebd4d --- /dev/null +++ b/pkg/controller/conversion/functions_test.go @@ -0,0 +1,103 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "fmt" + "testing" + + xpresource "github.com/crossplane/crossplane-runtime/pkg/resource" + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + + "github.com/crossplane/upjet/pkg/config" + "github.com/crossplane/upjet/pkg/config/conversion" + "github.com/crossplane/upjet/pkg/resource" + "github.com/crossplane/upjet/pkg/resource/fake" +) + +const ( + key1 = "key1" + val1 = "val1" + key2 = "key2" + val2 = "val2" + commonKey = "commonKey" + commonVal = "commonVal" +) + +func TestRoundTrip(t *testing.T) { + type args struct { + dst resource.Terraformed + src resource.Terraformed + conversions []conversion.Conversion + } + type want struct { + err error + dst resource.Terraformed + } + tests := map[string]struct { + reason string + args args + want want + }{ + "SuccessfulRoundTrip": { + reason: "Source object is successfully copied into the target object.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(fake.WithParameters(fake.NewMap(key1, val1))), + }, + want: want{ + dst: fake.NewTerraformed(fake.WithParameters(fake.NewMap(key1, val1))), + }, + }, + "SuccessfulRoundTripWithConversions": { + reason: "Source object is successfully converted into the target object with a set of conversions.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(fake.WithParameters(fake.NewMap(commonKey, commonVal, key1, val1))), + conversions: []conversion.Conversion{ + // Because the parameters of the fake.Terraformed is an unstructured + // map, all the fields of source (including key1) are successfully + // copied into dst by registry.RoundTrip. + // This conversion deletes the copied key "key1". + conversion.NewCustomConverter(conversion.AllVersions, conversion.AllVersions, func(_, target xpresource.Managed) error { + tr := target.(*fake.Terraformed) + delete(tr.Parameters, key1) + return nil + }), + conversion.NewFieldRenameConversion(conversion.AllVersions, fmt.Sprintf("parameterizable.parameters.%s", key1), conversion.AllVersions, fmt.Sprintf("parameterizable.parameters.%s", key2)), + }, + }, + want: want{ + dst: fake.NewTerraformed(fake.WithParameters(fake.NewMap(commonKey, commonVal, key2, val1))), + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + p := &config.Provider{ + Resources: map[string]*config.Resource{ + tc.args.dst.GetTerraformResourceType(): { + Conversions: tc.args.conversions, + }, + }, + } + r := ®istry{} + if err := r.RegisterConversions(p); err != nil { + t.Fatalf("\n%s\nRegisterConversions(p): Failed to register the conversions with the registry.\n", tc.reason) + } + err := r.RoundTrip(tc.args.dst, tc.args.src) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nRoundTrip(dst, src): -wantErr, +gotErr:\n%s", tc.reason, diff) + } + if tc.want.err != nil { + return + } + if diff := cmp.Diff(tc.want.dst, tc.args.dst); diff != "" { + t.Errorf("\n%s\nRoundTrip(dst, src): -wantDst, +gotDst:\n%s", tc.reason, diff) + } + }) + } +} diff --git a/pkg/controller/conversion/registry.go b/pkg/controller/conversion/registry.go index 6412c082..ab1fac5b 100644 --- a/pkg/controller/conversion/registry.go +++ b/pkg/controller/conversion/registry.go @@ -24,23 +24,37 @@ type registry struct { } // RegisterConversions registers the API version conversions from the specified -// provider configuration. -func RegisterConversions(provider *config.Provider) error { - if instance != nil { +// provider configuration with this registry. +func (r *registry) RegisterConversions(provider *config.Provider) error { + if r.provider != nil { return errors.New(errAlreadyRegistered) } - instance = ®istry{ - provider: provider, - } + r.provider = provider return nil } -// GetConversions returns the conversion.Conversions registered for the -// Terraformed resource. -func GetConversions(tr resource.Terraformed) []conversion.Conversion { +// GetConversions returns the conversion.Conversions registered in this +// registry for the specified Terraformed resource. +func (r *registry) GetConversions(tr resource.Terraformed) []conversion.Conversion { t := tr.GetTerraformResourceType() - if instance == nil || instance.provider == nil || instance.provider.Resources[t] == nil { + if r == nil || r.provider == nil || r.provider.Resources[t] == nil { return nil } - return instance.provider.Resources[t].Conversions + return r.provider.Resources[t].Conversions +} + +// GetConversions returns the conversion.Conversions registered for the +// specified Terraformed resource. +func GetConversions(tr resource.Terraformed) []conversion.Conversion { + return instance.GetConversions(tr) +} + +// RegisterConversions registers the API version conversions from the specified +// provider configuration. +func RegisterConversions(provider *config.Provider) error { + if instance != nil { + return errors.New(errAlreadyRegistered) + } + instance = ®istry{} + return instance.RegisterConversions(provider) } diff --git a/pkg/resource/fake/terraformed.go b/pkg/resource/fake/terraformed.go index ed71b356..ed891bb4 100644 --- a/pkg/resource/fake/terraformed.go +++ b/pkg/resource/fake/terraformed.go @@ -122,3 +122,33 @@ func (t *Terraformed) DeepCopyObject() runtime.Object { _ = json.Unmarshal(j, out) return out } + +// Option is an option to modify the properties of a Terraformed object. +type Option func(terraformed *Terraformed) + +// WithParameters sets the parameters of a Terraformed. +func WithParameters(params map[string]any) Option { + return func(tr *Terraformed) { + tr.Parameters = params + } +} + +// NewTerraformed initializes a new Terraformed with the given options. +func NewTerraformed(opts ...Option) *Terraformed { + tr := &Terraformed{} + for _, o := range opts { + o(tr) + } + return tr +} + +// NewMap prepares a map from the supplied key value parameters. +// The parameters slice must be a sequence of key, value pairs and must have +// an even length. The function will panic otherwise. +func NewMap(keyValue ...string) map[string]any { + m := make(map[string]any, len(keyValue)/2) + for i := 0; i < len(keyValue)-1; i += 2 { + m[keyValue[i]] = keyValue[i+1] + } + return m +}