Skip to content

Commit

Permalink
Fix resolutionrequest conversion
Browse files Browse the repository at this point in the history 
This commit addresses two issues with ResolutionRequest conversion.
First, it updates the "hubVersion" to v1alpha1, since v1alpha1 is the version that
supports conversion between versions, and addresses an incorrect comment about
what "hubVersion" represents.

Second, it adds conversion for ResolutionRequest statuses.
It updates tests to test the round trip conversion instead of one-way conversion,
as this helps avoid bugs with copy-paste.
  • Loading branch information
@lbernick @tekton-robot
lbernick authored and tekton-robot committed Apr 11, 2023
1 parent f69972a commit 14d4a98
Show file tree
Hide file tree
Showing 3 changed files with 166 additions and 148 deletions.
6 changes: 4 additions & 2 deletions cmd/webhook/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,9 @@ func newConversionController(ctx context.Context, cmw configmap.Watcher) *contro
"/resource-conversion",

// Specify the types of custom resource definitions that should be converted
// "HubVersion" is the stored version, and "Zygotes" are the supported versions
// "HubVersion" specifies which version of the CustomResource supports
// conversions to and from all types.
// "Zygotes" are the supported versions.
map[schema.GroupKind]conversion.GroupKindConversion{
v1.Kind("Task"): {
DefinitionName: pipeline.TaskResource.String(),
Expand Down Expand Up @@ -202,7 +204,7 @@ func newConversionController(ctx context.Context, cmw configmap.Watcher) *contro
},
resolutionv1beta1.Kind("ResolutionRequest"): {
DefinitionName: resolution.ResolutionRequestResource.String(),
HubVersion: resolutionv1beta1GroupVersion,
HubVersion: resolutionv1alpha1GroupVersion,
Zygotes: map[string]conversion.ConvertibleObject{
resolutionv1alpha1GroupVersion: &resolutionv1alpha1.ResolutionRequest{},
resolutionv1beta1GroupVersion: &resolutionv1beta1.ResolutionRequest{},
Expand Down
45 changes: 45 additions & 0 deletions pkg/apis/resolution/v1alpha1/resolution_request_conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func (rr *ResolutionRequest) ConvertTo(ctx context.Context, sink apis.Convertibl
switch sink := sink.(type) {
case *v1beta1.ResolutionRequest:
sink.ObjectMeta = rr.ObjectMeta
rr.Status.convertTo(ctx, &sink.Status)
return rr.Spec.ConvertTo(ctx, &sink.Spec)
default:
return fmt.Errorf("unknown version, got: %T", sink)
Expand All @@ -58,6 +59,22 @@ func (rrs *ResolutionRequestSpec) ConvertTo(ctx context.Context, sink *v1beta1.R
return nil
}

// convertTo converts a v1alpha1.ResolutionRequestStatus to a v1beta1.ResolutionRequestStatus
func (rrs *ResolutionRequestStatus) convertTo(ctx context.Context, sink *v1beta1.ResolutionRequestStatus) {
sink.Data = rrs.Data
if rrs.RefSource != nil {
refSource := pipelinev1beta1.RefSource{}
refSource.URI = rrs.RefSource.URI
refSource.EntryPoint = rrs.RefSource.EntryPoint
digest := make(map[string]string)
for k, v := range rrs.RefSource.Digest {
digest[k] = v
}
refSource.Digest = digest
sink.RefSource = &refSource
}
}

// ConvertFrom implements apis.Convertible
func (rr *ResolutionRequest) ConvertFrom(ctx context.Context, from apis.Convertible) error {
if apis.IsInDelete(ctx) {
Expand All @@ -66,6 +83,7 @@ func (rr *ResolutionRequest) ConvertFrom(ctx context.Context, from apis.Converti
switch from := from.(type) {
case *v1beta1.ResolutionRequest:
rr.ObjectMeta = from.ObjectMeta
rr.Status.convertFrom(ctx, &from.Status)
return rr.Spec.ConvertFrom(ctx, &from.Spec)
default:
return fmt.Errorf("unknown version, got: %T", from)
Expand Down Expand Up @@ -93,3 +111,30 @@ func (rrs *ResolutionRequestSpec) ConvertFrom(ctx context.Context, from *v1beta1

return nil
}

// convertTo converts a v1alpha1.ResolutionRequestStatus to a v1beta1.ResolutionRequestStatus
func (rrs *ResolutionRequestStatus) convertFrom(ctx context.Context, from *v1beta1.ResolutionRequestStatus) {
rrs.Data = from.Data

if from.RefSource != nil {
refSource := pipelinev1beta1.RefSource{}
refSource.URI = from.RefSource.URI
refSource.EntryPoint = from.RefSource.EntryPoint
digest := make(map[string]string)
for k, v := range from.RefSource.Digest {
digest[k] = v
}
refSource.Digest = digest
rrs.RefSource = &refSource
} else if from.Source != nil {
refSource := pipelinev1beta1.RefSource{}
refSource.URI = from.Source.URI
refSource.EntryPoint = from.Source.EntryPoint
digest := make(map[string]string)
for k, v := range from.Source.Digest {
digest[k] = v
}
refSource.Digest = digest
rrs.RefSource = &refSource
}
}
263 changes: 117 additions & 146 deletions pkg/apis/resolution/v1alpha1/resolution_request_conversion_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package v1alpha1

import (
"context"
"errors"
"testing"

"github.com/google/go-cmp/cmp"
Expand All @@ -42,182 +41,154 @@ func TestResolutionRequestConversionBadType(t *testing.T) {
}
}

func TestResolutionRequestConvertTo(t *testing.T) {
versions := []apis.Convertible{&v1beta1.ResolutionRequest{}}

func TestResolutionRequestConvertRoundTrip(t *testing.T) {
testCases := []struct {
name string
in *ResolutionRequest
out apis.Convertible
}{
{
name: "no params",
in: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: ResolutionRequestSpec{
Parameters: nil,
},
}{{
name: "no params",
in: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
out: &v1beta1.ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: v1beta1.ResolutionRequestSpec{
Params: nil,
Spec: ResolutionRequestSpec{
Parameters: nil,
},
},
}, {
name: "with params",
in: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: ResolutionRequestSpec{
Parameters: map[string]string{
"some-param": "some-value",
},
},
}, {
name: "with params",
in: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
}, {
name: "with status refsource",
in: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: ResolutionRequestSpec{
Parameters: map[string]string{
"some-param": "some-value",
},
Spec: ResolutionRequestSpec{
Parameters: map[string]string{
"some-param": "some-value",
},
Status: ResolutionRequestStatus{
ResolutionRequestStatusFields: ResolutionRequestStatusFields{
Data: "foobar",
RefSource: &pipelinev1beta1.RefSource{
URI: "abcd",
Digest: map[string]string{"123": "456"},
EntryPoint: "baz",
},
},
},
out: &v1beta1.ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
}, {
name: "with status, no refsource",
in: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: ResolutionRequestSpec{
Parameters: map[string]string{
"some-param": "some-value",
},
Spec: v1beta1.ResolutionRequestSpec{
Params: []pipelinev1beta1.Param{{
Name: "some-param",
Value: *pipelinev1beta1.NewStructuredValues("some-value"),
}},
},
Status: ResolutionRequestStatus{
ResolutionRequestStatusFields: ResolutionRequestStatusFields{
Data: "foobar",
},
},
},
}
}}

for _, tc := range testCases {
for _, version := range versions {
t.Run(tc.name, func(t *testing.T) {
got := version
if err := tc.in.ConvertTo(context.Background(), got); err != nil {
t.Fatalf("ConvertTo() = %v", err)
}
t.Logf("ConvertTo() = %#v", got)
if d := cmp.Diff(tc.out, got); d != "" {
t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d))
}
})
}
t.Run(tc.name, func(t *testing.T) {
got := &v1beta1.ResolutionRequest{}
if err := tc.in.ConvertTo(context.Background(), got); err != nil {
t.Fatalf("ConvertTo() = %v", err)
}

t.Logf("ConvertTo() = %#v", got)

roundTrip := &ResolutionRequest{}
if err := roundTrip.ConvertFrom(context.Background(), got); err != nil {
t.Errorf("ConvertFrom() = %v", err)
}

if d := cmp.Diff(tc.in, roundTrip); d != "" {
t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d))
}
})
}
}

func TestResolutionRequestConvertFrom(t *testing.T) {
versions := []apis.Convertible{&ResolutionRequest{}}

func TestResolutionRequestConvertFromDeprecated(t *testing.T) {
testCases := []struct {
name string
in apis.Convertible
out *ResolutionRequest
expectedErr error
}{
{
name: "no params",
in: &v1beta1.ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: v1beta1.ResolutionRequestSpec{
Params: nil,
},
},
out: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: ResolutionRequestSpec{
Parameters: nil,
},
name string
in *v1beta1.ResolutionRequest
want apis.Convertible
}{{
name: "with status.source",
in: &v1beta1.ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
}, {
name: "with only string params",
in: &v1beta1.ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: v1beta1.ResolutionRequestSpec{
Params: []pipelinev1beta1.Param{{
Name: "some-param",
Value: *pipelinev1beta1.NewStructuredValues("some-value"),
}},
},
Spec: v1beta1.ResolutionRequestSpec{
Params: nil,
},
out: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: ResolutionRequestSpec{
Parameters: map[string]string{
"some-param": "some-value",
Status: v1beta1.ResolutionRequestStatus{
ResolutionRequestStatusFields: v1beta1.ResolutionRequestStatusFields{
Source: &pipelinev1beta1.ConfigSource{
URI: "abcd",
Digest: map[string]string{"123": "456"},
EntryPoint: "baz",
},
},
},
}, {
name: "with non-string params",
in: &v1beta1.ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: v1beta1.ResolutionRequestSpec{
Params: []pipelinev1beta1.Param{
{
Name: "array-val",
Value: *pipelinev1beta1.NewStructuredValues("one", "two"),
}, {
Name: "string-val",
Value: *pipelinev1beta1.NewStructuredValues("a-string"),
}, {
Name: "object-val",
Value: *pipelinev1beta1.NewObject(map[string]string{
"key-one": "value-one",
"key-two": "value-two",
}),
},
},
want: &ResolutionRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
},
Spec: ResolutionRequestSpec{
Parameters: nil,
},
Status: ResolutionRequestStatus{
ResolutionRequestStatusFields: ResolutionRequestStatusFields{
RefSource: &pipelinev1beta1.RefSource{
URI: "abcd",
Digest: map[string]string{"123": "456"},
EntryPoint: "baz",
},
},
},
out: nil,
expectedErr: errors.New("cannot convert v1beta1 to v1alpha, non-string type parameter(s) found: array-val, object-val"),
},
}
}}

for _, tc := range testCases {
for _, version := range versions {
t.Run(tc.name, func(t *testing.T) {
got := version
err := got.ConvertFrom(context.Background(), tc.in)
if tc.expectedErr != nil {
if err == nil {
t.Fatalf("expected error '%s', but did not get an error", tc.expectedErr.Error())
} else if d := cmp.Diff(tc.expectedErr.Error(), err.Error()); d != "" {
t.Fatalf("error did not meet expected: %s", diff.PrintWantGot(d))
}
return
} else if err != nil {
t.Fatalf("ConvertFrom() = %v", err)
}
t.Logf("ConvertFrom() = %#v", got)
if d := cmp.Diff(tc.out, got); d != "" {
t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d))
}
})
}
t.Run(tc.name, func(t *testing.T) {
got := &ResolutionRequest{}
if err := got.ConvertFrom(context.Background(), tc.in); err != nil {
t.Errorf("ConvertFrom() = %v", err)
}

if d := cmp.Diff(tc.want, got); d != "" {
t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d))
}
})
}
}

0 comments on commit 14d4a98

Please sign in to comment.