diff --git a/pkg/config/conversion/conversions.go b/pkg/config/conversion/conversions.go index e25c42cd..6beb91d8 100644 --- a/pkg/config/conversion/conversions.go +++ b/pkg/config/conversion/conversions.go @@ -272,10 +272,14 @@ func newIdentityConversion(sourceVersion, targetVersion string, excludePaths ... // NewIdentityConversionExpandPaths returns a new Conversion from the specified // sourceVersion of an API to the specified targetVersion, which copies the // identical paths from the source to the target. excludePaths can be used -// to ignore certain field paths while copying. -// The field paths in excludePath are sorted in lexical order and expanded -// by default into spec.forProvider, spec.initProvider and -// status.atProvider paths. +// to ignore certain field paths while copying. Exclude paths must be specified +// in standard crossplane-runtime fieldpath library syntax, i.e., with proper +// indices for traversing map and slice types (e.g., a.b[*].c). +// The field paths in excludePaths are sorted in lexical order and are prefixed +// with each of the path prefixes specified with pathPrefixes. So if an +// exclude path "x" is specified with the prefix slice ["a", "b"], then +// paths a.x and b.x will both be skipped while copying fields from a source to +// a target. func NewIdentityConversionExpandPaths(sourceVersion, targetVersion string, pathPrefixes []string, excludePaths ...string) Conversion { return newIdentityConversion(sourceVersion, targetVersion, ExpandParameters(pathPrefixes, excludePaths...)...) } @@ -296,3 +300,10 @@ func ExpandParameters(prefixes []string, excludePaths ...string) []string { } return r } + +// DefaultPathPrefixes returns the list of the default path prefixes for +// excluding paths in the identity conversion. The returned value is +// ["spec.forProvider", "spec.initProvider", "status.atProvider"]. +func DefaultPathPrefixes() []string { + return []string{"spec.forProvider", "spec.initProvider", "status.atProvider"} +} diff --git a/pkg/config/conversion/conversions_test.go b/pkg/config/conversion/conversions_test.go index 03de9080..e818de86 100644 --- a/pkg/config/conversion/conversions_test.go +++ b/pkg/config/conversion/conversions_test.go @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// SPDX-FileCopyrightText: 2024 The Crossplane Authors // // SPDX-License-Identifier: Apache-2.0 @@ -6,11 +6,16 @@ package conversion import ( "fmt" + "slices" "testing" "github.com/crossplane/crossplane-runtime/pkg/fieldpath" + "github.com/crossplane/crossplane-runtime/pkg/resource" + "github.com/crossplane/crossplane-runtime/pkg/resource/fake" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/google/go-cmp/cmp" + jsoniter "github.com/json-iterator/go" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" ) @@ -136,6 +141,360 @@ func TestConvertPaved(t *testing.T) { } } +func TestIdentityConversion(t *testing.T) { + type args struct { + sourceVersion string + source resource.Managed + targetVersion string + target *mockManaged + pathPrefixes []string + excludePaths []string + } + type want struct { + converted bool + err error + target *mockManaged + } + tests := map[string]struct { + reason string + args args + want want + }{ + "SuccessfulConversionNoExclusions": { + reason: "Successfully copy identical fields from the source to the target with no exclusions.", + args: args{ + sourceVersion: AllVersions, + source: newMockManaged(map[string]any{ + "k1": "v1", + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }), + targetVersion: AllVersions, + target: newMockManaged(nil), + }, + want: want{ + converted: true, + target: newMockManaged(map[string]any{ + "k1": "v1", + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }), + }, + }, + "SuccessfulConversionExclusionsWithNoPrefixes": { + reason: "Successfully copy identical fields from the source to the target with exclusions without prefixes.", + args: args{ + sourceVersion: AllVersions, + source: newMockManaged(map[string]any{ + "k1": "v1", + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }), + targetVersion: AllVersions, + target: newMockManaged(nil), + excludePaths: []string{"k2", "k3"}, + }, + want: want{ + converted: true, + target: newMockManaged(map[string]any{ + "k1": "v1", + }), + }, + }, + "SuccessfulConversionNestedExclusionsWithNoPrefixes": { + reason: "Successfully copy identical fields from the source to the target with nested exclusions without prefixes.", + args: args{ + sourceVersion: AllVersions, + source: newMockManaged(map[string]any{ + "k1": "v1", + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }), + targetVersion: AllVersions, + target: newMockManaged(nil), + excludePaths: []string{"k2", "k3.nk1"}, + }, + want: want{ + converted: true, + target: newMockManaged(map[string]any{ + "k1": "v1", + // key key3 is copied without its nested element (as an empty map) + "k3": map[string]any{}, + }), + }, + }, + "SuccessfulConversionWithListExclusion": { + reason: "Successfully copy identical fields from the source to the target with an exclusion for a root-level list.", + args: args{ + sourceVersion: AllVersions, + source: newMockManaged(map[string]any{ + "k1": "v1", + "k2": []map[string]any{ + { + "nk3": "nv3", + }, + }, + }), + targetVersion: AllVersions, + target: newMockManaged(nil), + excludePaths: []string{"k2"}, + }, + want: want{ + converted: true, + target: newMockManaged(map[string]any{ + "k1": "v1", + }), + }, + }, + "SuccessfulConversionWithNestedListExclusion": { + reason: "Successfully copy identical fields from the source to the target with an exclusion for a nested list.", + args: args{ + sourceVersion: AllVersions, + source: newMockManaged(map[string]any{ + "k1": "v1", + "k2": []map[string]any{ + { + "nk3": []map[string]any{ + { + "nk4": "nv4", + }, + }, + }, + }, + }), + targetVersion: AllVersions, + target: newMockManaged(nil), + excludePaths: []string{"k2[*].nk3"}, + }, + want: want{ + converted: true, + target: newMockManaged(map[string]any{ + "k1": "v1", + "k2": []any{map[string]any{}}, + }), + }, + }, + "SuccessfulConversionWithDefaultExclusionPrefixes": { + reason: "Successfully copy identical fields from the source to the target with an exclusion for a nested list.", + args: args{ + sourceVersion: AllVersions, + source: newMockManaged(map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "k1": "v1", + "k2": "v2", + }, + "forProvider": map[string]any{ + "k1": "v1", + "k2": "v2", + }, + }, + "status": map[string]any{ + "atProvider": map[string]any{ + "k1": "v1", + "k2": "v2", + }, + }, + }), + targetVersion: AllVersions, + target: newMockManaged(nil), + excludePaths: []string{"k2"}, + pathPrefixes: DefaultPathPrefixes(), + }, + want: want{ + converted: true, + target: newMockManaged(map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "k1": "v1", + }, + "forProvider": map[string]any{ + "k1": "v1", + }, + }, + "status": map[string]any{ + "atProvider": map[string]any{ + "k1": "v1", + }, + }, + }), + }, + }, + } + for n, tc := range tests { + t.Run(n, func(t *testing.T) { + c := NewIdentityConversionExpandPaths(tc.args.sourceVersion, tc.args.targetVersion, tc.args.pathPrefixes, tc.args.excludePaths...) + converted, err := c.(*identityConversion).ConvertManaged(tc.args.source, tc.args.target) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nConvertManaged(source, target): -wantErr, +gotErr:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.converted, converted); diff != "" { + t.Errorf("\n%s\nConvertManaged(source, target): -wantConverted, +gotConverted:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.target.UnstructuredContent(), tc.args.target.UnstructuredContent()); diff != "" { + t.Errorf("\n%s\nConvertManaged(source, target): -wantTarget, +gotTarget:\n%s", tc.reason, diff) + } + }) + } +} + +func TestDefaultPathPrefixes(t *testing.T) { + // no need for a table-driven test here as we assert all the parameter roots + // in the MR schema are asserted. + want := []string{"spec.forProvider", "spec.initProvider", "status.atProvider"} + slices.Sort(want) + got := DefaultPathPrefixes() + slices.Sort(got) + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("DefaultPathPrefixes(): -want, +got:\n%s", diff) + } +} + +func TestSingletonListConversion(t *testing.T) { + type args struct { + sourceVersion string + sourceMap map[string]any + targetVersion string + targetMap map[string]any + crdPaths []string + mode Mode + } + type want struct { + converted bool + err error + targetMap map[string]any + } + tests := map[string]struct { + reason string + args args + want want + }{ + "SuccessfulToEmbeddedObjectConversion": { + reason: "Successful conversion from a singleton list to an embedded object.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + crdPaths: []string{"l"}, + mode: ToEmbeddedObject, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + }, + }, + }, + }, + "SuccessfulToSingletonListConversion": { + reason: "Successful conversion from an embedded object to a singleton list.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "o": map[string]any{ + "k": "v", + }, + }, + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + crdPaths: []string{"o"}, + mode: ToSingletonList, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "o": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + }, + }, + "NoCRDPath": { + reason: "No conversion when the specified CRD paths is empty.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "o": map[string]any{ + "k": "v", + }, + }, + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + mode: ToSingletonList, + }, + want: want{ + converted: false, + targetMap: map[string]any{}, + }, + }, + } + for n, tc := range tests { + t.Run(n, func(t *testing.T) { + c := NewSingletonListConversion(tc.args.sourceVersion, tc.args.targetVersion, tc.args.crdPaths, tc.args.mode) + sourceMap, err := roundTrip(tc.args.sourceMap) + if err != nil { + t.Fatalf("Failed to preprocess tc.args.sourceMap: %v", err) + } + targetMap, err := roundTrip(tc.args.targetMap) + if err != nil { + t.Fatalf("Failed to preprocess tc.args.targetMap: %v", err) + } + converted, err := c.(*singletonListConverter).ConvertPaved(fieldpath.Pave(sourceMap), fieldpath.Pave(targetMap)) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nConvertPaved(source, target): -wantErr, +gotErr:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.converted, converted); diff != "" { + t.Errorf("\n%s\nConvertPaved(source, target): -wantConverted, +gotConverted:\n%s", tc.reason, diff) + } + m, err := roundTrip(tc.want.targetMap) + if err != nil { + t.Fatalf("Failed to preprocess tc.want.targetMap: %v", err) + } + if diff := cmp.Diff(m, targetMap); diff != "" { + t.Errorf("\n%s\nConvertPaved(source, target): -wantTarget, +gotTarget:\n%s", tc.reason, diff) + } + }) + } +} + func getPaved(version, field string, value *string) *fieldpath.Paved { m := map[string]any{ "apiVersion": fmt.Sprintf("mockgroup/%s", version), @@ -146,3 +505,30 @@ func getPaved(version, field string, value *string) *fieldpath.Paved { } return fieldpath.Pave(m) } + +type mockManaged struct { + *fake.Managed + *fieldpath.Paved +} + +func (m *mockManaged) DeepCopyObject() runtime.Object { + buff, err := jsoniter.ConfigCompatibleWithStandardLibrary.Marshal(m.Paved.UnstructuredContent()) + if err != nil { + panic(err) + } + var u map[string]any + if err := jsoniter.Unmarshal(buff, &u); err != nil { + panic(err) + } + return &mockManaged{ + Managed: m.Managed.DeepCopyObject().(*fake.Managed), + Paved: fieldpath.Pave(u), + } +} + +func newMockManaged(m map[string]any) *mockManaged { + return &mockManaged{ + Managed: &fake.Managed{}, + Paved: fieldpath.Pave(m), + } +} diff --git a/pkg/config/conversion/runtime_conversion.go b/pkg/config/conversion/runtime_conversion.go index d3b22016..1d224167 100644 --- a/pkg/config/conversion/runtime_conversion.go +++ b/pkg/config/conversion/runtime_conversion.go @@ -5,6 +5,7 @@ package conversion import ( + "reflect" "slices" "sort" "strings" @@ -29,6 +30,11 @@ const ( ToSingletonList ) +const ( + errFmtMultiItemList = "singleton list, at the field path %s, must have a length of at most 1 but it has a length of %d" + errFmtNonSlice = "value at the field path %s must be []any, not %q" +) + // String returns a string representation of the conversion mode. func (m Mode) String() string { switch m { @@ -104,9 +110,12 @@ func Convert(params map[string]any, paths []string, mode Mode) (map[string]any, if v != nil { newVal = map[string]any{} s, ok := v.([]any) - if !ok || len(s) > 1 { - // if len(s) is 0, then it's not a slice - return nil, errors.Errorf("singleton list, at the field path %s, must have a length of at most 1 but it has a length of %d", e, len(s)) + if !ok { + // then it's not a slice + return nil, errors.Errorf(errFmtNonSlice, e, reflect.TypeOf(v)) + } + if len(s) > 1 { + return nil, errors.Errorf(errFmtMultiItemList, e, len(s)) } if len(s) > 0 { newVal = s[0] diff --git a/pkg/config/conversion/runtime_conversion_test.go b/pkg/config/conversion/runtime_conversion_test.go new file mode 100644 index 00000000..1af9e31b --- /dev/null +++ b/pkg/config/conversion/runtime_conversion_test.go @@ -0,0 +1,332 @@ +// SPDX-FileCopyrightText: 2024 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "reflect" + "testing" + + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + jsoniter "github.com/json-iterator/go" + "github.com/pkg/errors" +) + +func TestConvert(t *testing.T) { + type args struct { + params map[string]any + paths []string + mode Mode + } + type want struct { + err error + params map[string]any + } + tests := map[string]struct { + reason string + args args + want want + }{ + "NilParamsAndPaths": { + reason: "Conversion on an nil map should not fail.", + args: args{}, + }, + "EmptyPaths": { + reason: "Empty conversion on a map should be an identity function.", + args: args{ + params: map[string]any{"a": "b"}, + }, + want: want{ + params: map[string]any{"a": "b"}, + }, + }, + "SingletonListToEmbeddedObject": { + reason: "Should successfully convert a singleton list at the root level to an embedded object.", + args: args{ + params: map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + paths: []string{"l"}, + mode: ToEmbeddedObject, + }, + want: want{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + }, + }, + "NestedSingletonListsToEmbeddedObjectsPathsInLexicalOrder": { + reason: "Should successfully convert the parent & nested singleton lists to embedded objects. Paths specified in lexical order.", + args: args{ + params: map[string]any{ + "parent": []map[string]any{ + { + "child": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + paths: []string{"parent", "parent[*].child"}, + mode: ToEmbeddedObject, + }, + want: want{ + params: map[string]any{ + "parent": map[string]any{ + "child": map[string]any{ + "k": "v", + }, + }, + }, + }, + }, + "NestedSingletonListsToEmbeddedObjectsPathsInReverseLexicalOrder": { + reason: "Should successfully convert the parent & nested singleton lists to embedded objects. Paths specified in reverse-lexical order.", + args: args{ + params: map[string]any{ + "parent": []map[string]any{ + { + "child": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + paths: []string{"parent[*].child", "parent"}, + mode: ToEmbeddedObject, + }, + want: want{ + params: map[string]any{ + "parent": map[string]any{ + "child": map[string]any{ + "k": "v", + }, + }, + }, + }, + }, + "EmbeddedObjectToSingletonList": { + reason: "Should successfully convert an embedded object at the root level to a singleton list.", + args: args{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + paths: []string{"l"}, + mode: ToSingletonList, + }, + want: want{ + params: map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + "NestedEmbeddedObjectsToSingletonListInLexicalOrder": { + reason: "Should successfully convert the parent & nested embedded objects to singleton lists. Paths are specified in lexical order.", + args: args{ + params: map[string]any{ + "parent": map[string]any{ + "child": map[string]any{ + "k": "v", + }, + }, + }, + paths: []string{"parent", "parent[*].child"}, + mode: ToSingletonList, + }, + want: want{ + params: map[string]any{ + "parent": []map[string]any{ + { + "child": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + }, + }, + "NestedEmbeddedObjectsToSingletonListInReverseLexicalOrder": { + reason: "Should successfully convert the parent & nested embedded objects to singleton lists. Paths are specified in reverse-lexical order.", + args: args{ + params: map[string]any{ + "parent": map[string]any{ + "child": map[string]any{ + "k": "v", + }, + }, + }, + paths: []string{"parent[*].child", "parent"}, + mode: ToSingletonList, + }, + want: want{ + params: map[string]any{ + "parent": []map[string]any{ + { + "child": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + }, + }, + "FailConversionOfAMultiItemList": { + reason: `Conversion of a multi-item list in mode "ToEmbeddedObject" should fail.`, + args: args{ + params: map[string]any{ + "l": []map[string]any{ + { + "k1": "v1", + }, + { + "k2": "v2", + }, + }, + }, + paths: []string{"l"}, + mode: ToEmbeddedObject, + }, + want: want{ + err: errors.Errorf(errFmtMultiItemList, "l", 2), + }, + }, + "FailConversionOfNonSlice": { + reason: `Conversion of a non-slice value in mode "ToEmbeddedObject" should fail.`, + args: args{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + paths: []string{"l"}, + mode: ToEmbeddedObject, + }, + want: want{ + err: errors.Errorf(errFmtNonSlice, "l", reflect.TypeOf(map[string]any{})), + }, + }, + "ToSingletonListWithNonExistentPath": { + reason: `"ToSingletonList" mode conversions specifying only non-existent paths should be identity functions.`, + args: args{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + paths: []string{"nonexistent"}, + mode: ToSingletonList, + }, + want: want{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + }, + }, + "ToEmbeddedObjectWithNonExistentPath": { + reason: `"ToEmbeddedObject" mode conversions specifying only non-existent paths should be identity functions.`, + args: args{ + params: map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + paths: []string{"nonexistent"}, + mode: ToEmbeddedObject, + }, + want: want{ + params: map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + } + + for n, tt := range tests { + t.Run(n, func(t *testing.T) { + params, err := roundTrip(tt.args.params) + if err != nil { + t.Fatalf("Failed to preprocess tt.args.params: %v", err) + } + wantParams, err := roundTrip(tt.want.params) + if err != nil { + t.Fatalf("Failed to preprocess tt.want.params: %v", err) + } + got, err := Convert(params, tt.args.paths, tt.args.mode) + if diff := cmp.Diff(tt.want.err, err, test.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nConvert(tt.args.params, tt.args.paths): -wantErr, +gotErr:\n%s", tt.reason, diff) + } + if diff := cmp.Diff(wantParams, got); diff != "" { + t.Errorf("\n%s\nConvert(tt.args.params, tt.args.paths): -wantConverted, +gotConverted:\n%s", tt.reason, diff) + } + }) + } +} + +func TestModeString(t *testing.T) { + tests := map[string]struct { + m Mode + want string + }{ + "ToSingletonList": { + m: ToSingletonList, + want: "toSingletonList", + }, + "ToEmbeddedObject": { + m: ToEmbeddedObject, + want: "toEmbeddedObject", + }, + "Unknown": { + m: ToSingletonList + 1, + want: "unknown", + }, + } + for n, tt := range tests { + t.Run(n, func(t *testing.T) { + if diff := cmp.Diff(tt.want, tt.m.String()); diff != "" { + t.Errorf("String(): -want, +got:\n%s", diff) + } + }) + } +} + +func roundTrip(m map[string]any) (map[string]any, error) { + if len(m) == 0 { + return m, nil + } + buff, err := jsoniter.ConfigCompatibleWithStandardLibrary.Marshal(m) + if err != nil { + return nil, err + } + var r map[string]any + return r, jsoniter.ConfigCompatibleWithStandardLibrary.Unmarshal(buff, &r) +} diff --git a/pkg/controller/conversion/functions.go b/pkg/controller/conversion/functions.go index e65d25b9..da4f46f5 100644 --- a/pkg/controller/conversion/functions.go +++ b/pkg/controller/conversion/functions.go @@ -13,6 +13,12 @@ import ( "github.com/crossplane/upjet/pkg/resource" ) +const ( + errFmtPrioritizedManagedConversion = "cannot apply the PrioritizedManagedConversion for the %q object" + errFmtPavedConversion = "cannot apply the PavedConversion for the %q object" + errFmtManagedConversion = "cannot apply the ManagedConversion for the %q object" +) + // 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 of this registry. @@ -21,7 +27,7 @@ func (r *registry) RoundTrip(dst, src resource.Terraformed) error { //nolint:goc for _, c := range r.GetConversions(dst) { if pc, ok := c.(conversion.PrioritizedManagedConversion); ok { if _, err := pc.ConvertManaged(src, dst); err != nil { - return errors.Wrapf(err, "cannot apply the PrioritizedManagedConversion for the %q object", dst.GetTerraformResourceType()) + return errors.Wrapf(err, errFmtPrioritizedManagedConversion, dst.GetTerraformResourceType()) } } } @@ -41,7 +47,7 @@ func (r *registry) RoundTrip(dst, src resource.Terraformed) error { //nolint:goc 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()) + return errors.Wrapf(err, errFmtPavedConversion, dst.GetTerraformResourceType()) } } } @@ -58,7 +64,7 @@ func (r *registry) RoundTrip(dst, src resource.Terraformed) error { //nolint:goc continue // then already run in the first stage } if _, err := tc.ConvertManaged(src, dst); err != nil { - return errors.Wrapf(err, "cannot apply the ManagedConversion for the %q object", dst.GetTerraformResourceType()) + return errors.Wrapf(err, errFmtManagedConversion, dst.GetTerraformResourceType()) } } } diff --git a/pkg/controller/conversion/functions_test.go b/pkg/controller/conversion/functions_test.go index 2c1213e3..2df01aee 100644 --- a/pkg/controller/conversion/functions_test.go +++ b/pkg/controller/conversion/functions_test.go @@ -8,9 +8,12 @@ import ( "fmt" "testing" + "github.com/crossplane/crossplane-runtime/pkg/fieldpath" xpresource "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/google/go-cmp/cmp" + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/runtime" "github.com/crossplane/upjet/pkg/config" "github.com/crossplane/upjet/pkg/config/conversion" @@ -25,6 +28,7 @@ const ( val2 = "val2" commonKey = "commonKey" commonVal = "commonVal" + errTest = "test error" ) func TestRoundTrip(t *testing.T) { @@ -76,6 +80,39 @@ func TestRoundTrip(t *testing.T) { dst: fake.NewTerraformed(fake.WithParameters(fake.NewMap(commonKey, commonVal, key2, val1))), }, }, + "RoundTripFailedPrioritizedConversion": { + reason: "Should return an error if a PrioritizedConversion fails.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(), + conversions: []conversion.Conversion{failedPrioritizedConversion{}}, + }, + want: want{ + err: errors.Wrapf(errors.New(errTest), errFmtPrioritizedManagedConversion, ""), + }, + }, + "RoundTripFailedPavedConversion": { + reason: "Should return an error if a PavedConversion fails.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(), + conversions: []conversion.Conversion{failedPavedConversion{}}, + }, + want: want{ + err: errors.Wrapf(errors.New(errTest), errFmtPavedConversion, ""), + }, + }, + "RoundTripFailedManagedConversion": { + reason: "Should return an error if a ManagedConversion fails.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(), + conversions: []conversion.Conversion{failedManagedConversion{}}, + }, + want: want{ + err: errors.Wrapf(errors.New(errTest), errFmtManagedConversion, ""), + }, + }, "RoundTripWithExcludedFields": { reason: "Source object is successfully copied into the target object with certain fields excluded.", args: args{ @@ -114,3 +151,35 @@ func TestRoundTrip(t *testing.T) { }) } } + +type failedPrioritizedConversion struct{} + +func (failedPrioritizedConversion) Applicable(_, _ runtime.Object) bool { + return true +} + +func (failedPrioritizedConversion) ConvertManaged(_, _ xpresource.Managed) (bool, error) { + return false, errors.New(errTest) +} + +func (failedPrioritizedConversion) Prioritized() {} + +type failedPavedConversion struct{} + +func (failedPavedConversion) Applicable(_, _ runtime.Object) bool { + return true +} + +func (failedPavedConversion) ConvertPaved(_, _ *fieldpath.Paved) (bool, error) { + return false, errors.New(errTest) +} + +type failedManagedConversion struct{} + +func (failedManagedConversion) Applicable(_, _ runtime.Object) bool { + return true +} + +func (failedManagedConversion) ConvertManaged(_, _ xpresource.Managed) (bool, error) { + return false, errors.New(errTest) +}