Skip to content

Commit 4681a33

Browse files
lbernicklbernick
committed
Fix resolutionrequest conversion
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.
1 parent fb38679 commit 4681a33

File tree

3 files changed

+168
-148
lines changed

3 files changed

+168
-148
lines changed

‎cmd/webhook/main.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,9 @@ func newConversionController(ctx context.Context, cmw configmap.Watcher) *contro
166166
"/resource-conversion",
167167

168168
// Specify the types of custom resource definitions that should be converted
169-
// "HubVersion" is the stored version, and "Zygotes" are the supported versions
169+
// "HubVersion" specifies which version of the CustomResource supports
170+
// conversions to and from all types.
171+
// "Zygotes" are the supported versions.
170172
map[schema.GroupKind]conversion.GroupKindConversion{
171173
v1.Kind("Task"): {
172174
DefinitionName: pipeline.TaskResource.String(),
@@ -202,7 +204,7 @@ func newConversionController(ctx context.Context, cmw configmap.Watcher) *contro
202204
},
203205
resolutionv1beta1.Kind("ResolutionRequest"): {
204206
DefinitionName: resolution.ResolutionRequestResource.String(),
205-
HubVersion: resolutionv1beta1GroupVersion,
207+
HubVersion: resolutionv1alpha1GroupVersion,
206208
Zygotes: map[string]conversion.ConvertibleObject{
207209
resolutionv1alpha1GroupVersion: &resolutionv1alpha1.ResolutionRequest{},
208210
resolutionv1beta1GroupVersion: &resolutionv1beta1.ResolutionRequest{},

‎pkg/apis/resolution/v1alpha1/resolution_request_conversion.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ func (rr *ResolutionRequest) ConvertTo(ctx context.Context, sink apis.Convertibl
3737
switch sink := sink.(type) {
3838
case *v1beta1.ResolutionRequest:
3939
sink.ObjectMeta = rr.ObjectMeta
40+
rr.Status.convertTo(ctx, &sink.Status)
4041
return rr.Spec.ConvertTo(ctx, &sink.Spec)
4142
default:
4243
return fmt.Errorf("unknown version, got: %T", sink)
@@ -58,6 +59,22 @@ func (rrs *ResolutionRequestSpec) ConvertTo(ctx context.Context, sink *v1beta1.R
5859
return nil
5960
}
6061

62+
// convertTo converts a v1alpha1.ResolutionRequestStatus to a v1beta1.ResolutionRequestStatus
63+
func (rrs *ResolutionRequestStatus) convertTo(ctx context.Context, sink *v1beta1.ResolutionRequestStatus) {
64+
sink.Data = rrs.Data
65+
if rrs.RefSource != nil {
66+
refSource := pipelinev1beta1.RefSource{}
67+
refSource.URI = rrs.RefSource.URI
68+
refSource.EntryPoint = rrs.RefSource.EntryPoint
69+
digest := make(map[string]string)
70+
for k, v := range rrs.RefSource.Digest {
71+
digest[k] = v
72+
}
73+
refSource.Digest = digest
74+
sink.RefSource = &refSource
75+
}
76+
}
77+
6178
// ConvertFrom implements apis.Convertible
6279
func (rr *ResolutionRequest) ConvertFrom(ctx context.Context, from apis.Convertible) error {
6380
if apis.IsInDelete(ctx) {
@@ -66,6 +83,7 @@ func (rr *ResolutionRequest) ConvertFrom(ctx context.Context, from apis.Converti
6683
switch from := from.(type) {
6784
case *v1beta1.ResolutionRequest:
6885
rr.ObjectMeta = from.ObjectMeta
86+
rr.Status.convertFrom(ctx, &from.Status)
6987
return rr.Spec.ConvertFrom(ctx, &from.Spec)
7088
default:
7189
return fmt.Errorf("unknown version, got: %T", from)
@@ -93,3 +111,30 @@ func (rrs *ResolutionRequestSpec) ConvertFrom(ctx context.Context, from *v1beta1
93111

94112
return nil
95113
}
114+
115+
// convertTo converts a v1alpha1.ResolutionRequestStatus to a v1beta1.ResolutionRequestStatus
116+
func (rrs *ResolutionRequestStatus) convertFrom(ctx context.Context, from *v1beta1.ResolutionRequestStatus) {
117+
rrs.Data = from.Data
118+
119+
if from.RefSource != nil {
120+
refSource := pipelinev1beta1.RefSource{}
121+
refSource.URI = from.RefSource.URI
122+
refSource.EntryPoint = from.RefSource.EntryPoint
123+
digest := make(map[string]string)
124+
for k, v := range from.RefSource.Digest {
125+
digest[k] = v
126+
}
127+
refSource.Digest = digest
128+
rrs.RefSource = &refSource
129+
} else if from.Source != nil {
130+
refSource := pipelinev1beta1.RefSource{}
131+
refSource.URI = from.Source.URI
132+
refSource.EntryPoint = from.Source.EntryPoint
133+
digest := make(map[string]string)
134+
for k, v := range from.Source.Digest {
135+
digest[k] = v
136+
}
137+
refSource.Digest = digest
138+
rrs.RefSource = &refSource
139+
}
140+
}

‎pkg/apis/resolution/v1alpha1/resolution_request_conversion_test.go‎

Lines changed: 119 additions & 146 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package v1alpha1
1919

2020
import (
2121
"context"
22-
"errors"
2322
"testing"
2423

2524
"github.com/google/go-cmp/cmp"
@@ -42,182 +41,156 @@ func TestResolutionRequestConversionBadType(t *testing.T) {
4241
}
4342
}
4443

45-
func TestResolutionRequestConvertTo(t *testing.T) {
46-
versions := []apis.Convertible{&v1beta1.ResolutionRequest{}}
47-
44+
func TestResolutionRequestConvertRoundTrip(t *testing.T) {
4845
testCases := []struct {
4946
name string
5047
in *ResolutionRequest
5148
out apis.Convertible
52-
}{
53-
{
54-
name: "no params",
55-
in: &ResolutionRequest{
56-
ObjectMeta: metav1.ObjectMeta{
57-
Name: "foo",
58-
Namespace: "bar",
59-
},
60-
Spec: ResolutionRequestSpec{
61-
Parameters: nil,
62-
},
49+
}{{
50+
name: "no params",
51+
in: &ResolutionRequest{
52+
ObjectMeta: metav1.ObjectMeta{
53+
Name: "foo",
54+
Namespace: "bar",
6355
},
64-
out: &v1beta1.ResolutionRequest{
65-
ObjectMeta: metav1.ObjectMeta{
66-
Name: "foo",
67-
Namespace: "bar",
68-
},
69-
Spec: v1beta1.ResolutionRequestSpec{
70-
Params: nil,
56+
Spec: ResolutionRequestSpec{
57+
Parameters: nil,
58+
},
59+
},
60+
}, {
61+
name: "with params",
62+
in: &ResolutionRequest{
63+
ObjectMeta: metav1.ObjectMeta{
64+
Name: "foo",
65+
Namespace: "bar",
66+
},
67+
Spec: ResolutionRequestSpec{
68+
Parameters: map[string]string{
69+
"some-param": "some-value",
7170
},
7271
},
73-
}, {
74-
name: "with params",
75-
in: &ResolutionRequest{
76-
ObjectMeta: metav1.ObjectMeta{
77-
Name: "foo",
78-
Namespace: "bar",
72+
},
73+
}, {
74+
name: "with status refsource",
75+
in: &ResolutionRequest{
76+
ObjectMeta: metav1.ObjectMeta{
77+
Name: "foo",
78+
Namespace: "bar",
79+
},
80+
Spec: ResolutionRequestSpec{
81+
Parameters: map[string]string{
82+
"some-param": "some-value",
7983
},
80-
Spec: ResolutionRequestSpec{
81-
Parameters: map[string]string{
82-
"some-param": "some-value",
84+
},
85+
Status: ResolutionRequestStatus{
86+
ResolutionRequestStatusFields: ResolutionRequestStatusFields{
87+
Data: "foobar",
88+
RefSource: &pipelinev1beta1.RefSource{
89+
URI: "abcd",
90+
Digest: map[string]string{"123": "456"},
91+
EntryPoint: "baz",
8392
},
8493
},
8594
},
86-
out: &v1beta1.ResolutionRequest{
87-
ObjectMeta: metav1.ObjectMeta{
88-
Name: "foo",
89-
Namespace: "bar",
95+
},
96+
}, {
97+
name: "with status, no refsource",
98+
in: &ResolutionRequest{
99+
ObjectMeta: metav1.ObjectMeta{
100+
Name: "foo",
101+
Namespace: "bar",
102+
},
103+
Spec: ResolutionRequestSpec{
104+
Parameters: map[string]string{
105+
"some-param": "some-value",
90106
},
91-
Spec: v1beta1.ResolutionRequestSpec{
92-
Params: []pipelinev1beta1.Param{{
93-
Name: "some-param",
94-
Value: *pipelinev1beta1.NewStructuredValues("some-value"),
95-
}},
107+
},
108+
Status: ResolutionRequestStatus{
109+
ResolutionRequestStatusFields: ResolutionRequestStatusFields{
110+
Data: "foobar",
96111
},
97112
},
98113
},
99-
}
114+
}}
100115

101116
for _, tc := range testCases {
102-
for _, version := range versions {
103-
t.Run(tc.name, func(t *testing.T) {
104-
got := version
105-
if err := tc.in.ConvertTo(context.Background(), got); err != nil {
106-
t.Fatalf("ConvertTo() = %v", err)
107-
}
108-
t.Logf("ConvertTo() = %#v", got)
109-
if d := cmp.Diff(tc.out, got); d != "" {
110-
t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d))
111-
}
112-
})
113-
}
117+
118+
t.Run(tc.name, func(t *testing.T) {
119+
got := &v1beta1.ResolutionRequest{}
120+
if err := tc.in.ConvertTo(context.Background(), got); err != nil {
121+
t.Fatalf("ConvertTo() = %v", err)
122+
}
123+
124+
t.Logf("ConvertTo() = %#v", got)
125+
126+
roundTrip := &ResolutionRequest{}
127+
if err := roundTrip.ConvertFrom(context.Background(), got); err != nil {
128+
t.Errorf("ConvertFrom() = %v", err)
129+
}
130+
131+
if d := cmp.Diff(tc.in, roundTrip); d != "" {
132+
t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d))
133+
}
134+
})
114135
}
115136
}
116137

117-
func TestResolutionRequestConvertFrom(t *testing.T) {
118-
versions := []apis.Convertible{&ResolutionRequest{}}
119-
138+
func TestResolutionRequestConvertFromDeprecated(t *testing.T) {
120139
testCases := []struct {
121-
name string
122-
in apis.Convertible
123-
out *ResolutionRequest
124-
expectedErr error
125-
}{
126-
{
127-
name: "no params",
128-
in: &v1beta1.ResolutionRequest{
129-
ObjectMeta: metav1.ObjectMeta{
130-
Name: "foo",
131-
Namespace: "bar",
132-
},
133-
Spec: v1beta1.ResolutionRequestSpec{
134-
Params: nil,
135-
},
136-
},
137-
out: &ResolutionRequest{
138-
ObjectMeta: metav1.ObjectMeta{
139-
Name: "foo",
140-
Namespace: "bar",
141-
},
142-
Spec: ResolutionRequestSpec{
143-
Parameters: nil,
144-
},
140+
name string
141+
in *v1beta1.ResolutionRequest
142+
want apis.Convertible
143+
}{{
144+
name: "with status.source",
145+
in: &v1beta1.ResolutionRequest{
146+
ObjectMeta: metav1.ObjectMeta{
147+
Name: "foo",
148+
Namespace: "bar",
145149
},
146-
}, {
147-
name: "with only string params",
148-
in: &v1beta1.ResolutionRequest{
149-
ObjectMeta: metav1.ObjectMeta{
150-
Name: "foo",
151-
Namespace: "bar",
152-
},
153-
Spec: v1beta1.ResolutionRequestSpec{
154-
Params: []pipelinev1beta1.Param{{
155-
Name: "some-param",
156-
Value: *pipelinev1beta1.NewStructuredValues("some-value"),
157-
}},
158-
},
150+
Spec: v1beta1.ResolutionRequestSpec{
151+
Params: nil,
159152
},
160-
out: &ResolutionRequest{
161-
ObjectMeta: metav1.ObjectMeta{
162-
Name: "foo",
163-
Namespace: "bar",
164-
},
165-
Spec: ResolutionRequestSpec{
166-
Parameters: map[string]string{
167-
"some-param": "some-value",
153+
Status: v1beta1.ResolutionRequestStatus{
154+
ResolutionRequestStatusFields: v1beta1.ResolutionRequestStatusFields{
155+
Source: &pipelinev1beta1.ConfigSource{
156+
URI: "abcd",
157+
Digest: map[string]string{"123": "456"},
158+
EntryPoint: "baz",
168159
},
169160
},
170161
},
171-
}, {
172-
name: "with non-string params",
173-
in: &v1beta1.ResolutionRequest{
174-
ObjectMeta: metav1.ObjectMeta{
175-
Name: "foo",
176-
Namespace: "bar",
177-
},
178-
Spec: v1beta1.ResolutionRequestSpec{
179-
Params: []pipelinev1beta1.Param{
180-
{
181-
Name: "array-val",
182-
Value: *pipelinev1beta1.NewStructuredValues("one", "two"),
183-
}, {
184-
Name: "string-val",
185-
Value: *pipelinev1beta1.NewStructuredValues("a-string"),
186-
}, {
187-
Name: "object-val",
188-
Value: *pipelinev1beta1.NewObject(map[string]string{
189-
"key-one": "value-one",
190-
"key-two": "value-two",
191-
}),
192-
},
162+
},
163+
want: &ResolutionRequest{
164+
ObjectMeta: metav1.ObjectMeta{
165+
Name: "foo",
166+
Namespace: "bar",
167+
},
168+
Spec: ResolutionRequestSpec{
169+
Parameters: nil,
170+
},
171+
Status: ResolutionRequestStatus{
172+
ResolutionRequestStatusFields: ResolutionRequestStatusFields{
173+
RefSource: &pipelinev1beta1.RefSource{
174+
URI: "abcd",
175+
Digest: map[string]string{"123": "456"},
176+
EntryPoint: "baz",
193177
},
194178
},
195179
},
196-
out: nil,
197-
expectedErr: errors.New("cannot convert v1beta1 to v1alpha, non-string type parameter(s) found: array-val, object-val"),
198180
},
199-
}
181+
}}
200182

201183
for _, tc := range testCases {
202-
for _, version := range versions {
203-
t.Run(tc.name, func(t *testing.T) {
204-
got := version
205-
err := got.ConvertFrom(context.Background(), tc.in)
206-
if tc.expectedErr != nil {
207-
if err == nil {
208-
t.Fatalf("expected error '%s', but did not get an error", tc.expectedErr.Error())
209-
} else if d := cmp.Diff(tc.expectedErr.Error(), err.Error()); d != "" {
210-
t.Fatalf("error did not meet expected: %s", diff.PrintWantGot(d))
211-
}
212-
return
213-
} else if err != nil {
214-
t.Fatalf("ConvertFrom() = %v", err)
215-
}
216-
t.Logf("ConvertFrom() = %#v", got)
217-
if d := cmp.Diff(tc.out, got); d != "" {
218-
t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d))
219-
}
220-
})
221-
}
184+
t.Run(tc.name, func(t *testing.T) {
185+
got := &ResolutionRequest{}
186+
if err := got.ConvertFrom(context.Background(), tc.in); err != nil {
187+
t.Errorf("ConvertFrom() = %v", err)
188+
}
189+
190+
if d := cmp.Diff(tc.want, got); d != "" {
191+
t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d))
192+
}
193+
})
194+
222195
}
223196
}

0 commit comments

Comments
 (0)