Make SubscribableSpec implement apis.Convertible#3472
Conversation
knative-prow-robot
added
do-not-merge/work-in-progress
aliok
force-pushed
the
duck_conv_02_SubscribableTypeSpec
branch
from
2c737f0 to
06d3ca1
Compare
aliok
force-pushed
the
duck_conv_02_SubscribableTypeSpec
branch
2 times, most recently
from
aa4d9b8 to
1452944
Compare
|
/retest |
1 similar comment
|
/retest |
knative-prow-robot
added
the
needs-rebase
aliok
force-pushed
the
duck_conv_02_SubscribableTypeSpec
branch
from
5cceb7c to
1ef897b
Compare
knative-prow-robot
removed
the
needs-rebase
aliok
changed the title
knative-prow-robot
removed
the
do-not-merge/work-in-progress
|
The following is the coverage report on the affected files.
|
| sink.Subscribable.Subscribers[i].ConvertFrom(ctx, s) | ||
| } | ||
| } | ||
| default: |
There was a problem hiding this comment.
Don't you need the conversion to v1 too here?
There was a problem hiding this comment.
I am going step by step. In this and 2 more PRs I just make things impelemtn the interface.
The v1alpha1<>v1 is to be added later in #3477
devguyio
left a comment
devguyio
left a comment
There was a problem hiding this comment.
Left a couple of questions.
| func TestSubscribableSpecConversionBadType(t *testing.T) { | ||
| good, bad := &SubscribableSpec{}, &SubscribableSpec{} | ||
|
|
||
| if err := good.ConvertTo(context.Background(), bad); err == nil { |
There was a problem hiding this comment.
is bad considered bad cause this is a conversion from v1 to v1 ? and the good case would be from a different version to v1 ?
There was a problem hiding this comment.
yes. that's the convention used in everywhere in knative/eventing btw.
|
|
||
| // ConvertTo implements apis.Convertible | ||
| func (source *SubscribableSpec) ConvertTo(ctx context.Context, sink apis.Convertible) error { | ||
| return fmt.Errorf("v1 is the highest known version, got: %T", sink) |
There was a problem hiding this comment.
Why is calling any Convertible method considered wrong? can't we convert downwards? v1obj.convertTo(v1beta1obj) ? and if so, how come the v1alpha1 have a convertFrom ? isn't that used for downgrade conversion?
There was a problem hiding this comment.
There are 2 methods on apis.Convertible:
- ConvertTo: sets value of other from this
- ConvertFrom: sets value of this from other
The code for conversion is kept on the lower version types. It is how it is in all over Knative and I like this practice.
To convert down, you would need to do v1beta1obj.ConvertFrom(v1obj).
how come the v1alpha1 have a convertFrom
if your storage version is v1alpha1, you need to convert v1 object to v1alpha1. That's why that method is needed.
|
/approve Waiting for last lgtm from @devguyio |
knative-prow-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aliok, devguyio, slinkydeveloper The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests: |
|
/retest |
7 participants
Part of #3474
Same story with that #3471, but for
SubscribableSpec