Skip to content

Make SubscribableSpec implement apis.Convertible #3472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knative-prow-robot merged 4 commits into from
Jul 6, 2020
Merged

Make SubscribableSpec implement apis.Convertible #3472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
547292c
Make SubscribableSpec implement apis.Convertible
aliok Jul 3, 2020
4dd737d
Tests for bad types
aliok Jul 3, 2020
51720ea
Fix build
aliok Jul 3, 2020
1ef897b
Fix build
aliok Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/apis/duck/v1/subscribable_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ var (
_ apis.Listable = (*Subscribable)(nil)

_ apis.Convertible = (*Subscribable)(nil)
_ apis.Convertible = (*SubscribableSpec)(nil)
_ apis.Convertible = (*SubscribableStatus)(nil)
)

Expand Down
10 changes: 10 additions & 0 deletions pkg/apis/duck/v1/subscribable_types_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ func (sink *Subscribable) ConvertFrom(ctx context.Context, source apis.Convertib
return fmt.Errorf("v1 is the highest known version, got: %T", source)
}

// 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)
Copy link
Copy Markdown
Contributor

@devguyio devguyio Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

@aliok aliok Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// ConvertFrom implements apis.Convertible
func (sink *SubscribableSpec) ConvertFrom(ctx context.Context, source apis.Convertible) error {
return fmt.Errorf("v1 is the highest known version, got: %T", source)
}

// ConvertTo implements apis.Convertible
func (source *SubscribableStatus) ConvertTo(ctx context.Context, sink apis.Convertible) error {
return fmt.Errorf("v1 is the highest known version, got: %T", sink)
Expand Down
12 changes: 12 additions & 0 deletions pkg/apis/duck/v1/subscribable_types_conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ func TestSubscribableConversionBadType(t *testing.T) {
}
}

func TestSubscribableSpecConversionBadType(t *testing.T) {
good, bad := &SubscribableSpec{}, &SubscribableSpec{}

if err := good.ConvertTo(context.Background(), bad); err == nil {
Copy link
Copy Markdown
Contributor

@devguyio devguyio Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Copy Markdown
Member Author

@aliok aliok Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. that's the convention used in everywhere in knative/eventing btw.

t.Errorf("ConvertTo() = %#v, wanted error", bad)
}

if err := good.ConvertFrom(context.Background(), bad); err == nil {
t.Errorf("ConvertFrom() = %#v, wanted error", good)
}
}

func TestSubscribableStatusConversionBadType(t *testing.T) {
good, bad := &SubscribableStatus{}, &SubscribableStatus{}

Expand Down
1 change: 1 addition & 0 deletions pkg/apis/duck/v1alpha1/subscribable_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ var (

_ apis.Convertible = (*SubscribableType)(nil)

_ apis.Convertible = (*SubscribableTypeSpec)(nil)
_ apis.Convertible = (*SubscribableTypeStatus)(nil)
)

Expand Down
41 changes: 26 additions & 15 deletions pkg/apis/duck/v1alpha1/subscribable_types_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,18 @@ func (source *SubscribableType) ConvertTo(ctx context.Context, obj apis.Converti
}
}

// ConvertTo helps implement apis.Convertible
func (source *SubscribableTypeSpec) ConvertTo(ctx context.Context, sink *duckv1beta1.SubscribableSpec) error {
if source.Subscribable != nil {
sink.Subscribers = make([]duckv1beta1.SubscriberSpec, len(source.Subscribable.Subscribers))
for i, s := range source.Subscribable.Subscribers {
s.ConvertTo(ctx, &sink.Subscribers[i])
// ConvertTo implements apis.Convertible
func (source *SubscribableTypeSpec) ConvertTo(ctx context.Context, obj apis.Convertible) error {
switch sink := obj.(type) {
case *duckv1beta1.SubscribableSpec:
if source.Subscribable != nil {
sink.Subscribers = make([]duckv1beta1.SubscriberSpec, len(source.Subscribable.Subscribers))
for i, s := range source.Subscribable.Subscribers {
s.ConvertTo(ctx, &sink.Subscribers[i])
}
}
default:
return fmt.Errorf("unknown version, got: %T", sink)
}
return nil
}
Expand Down Expand Up @@ -98,23 +103,29 @@ func (sink *SubscribableType) ConvertFrom(ctx context.Context, obj apis.Converti
case *duckv1beta1.Subscribable:
sink.ObjectMeta = source.ObjectMeta
sink.Status.ConvertFrom(ctx, &source.Status)
sink.Spec.ConvertFrom(ctx, source.Spec)
sink.Spec.ConvertFrom(ctx, &source.Spec)
return nil
default:
return fmt.Errorf("unknown version, got: %T", source)
}
}

// ConvertFrom helps implement apis.Convertible
func (sink *SubscribableTypeSpec) ConvertFrom(ctx context.Context, source duckv1beta1.SubscribableSpec) {
if len(source.Subscribers) > 0 {
sink.Subscribable = &Subscribable{
Subscribers: make([]SubscriberSpec, len(source.Subscribers)),
}
for i, s := range source.Subscribers {
sink.Subscribable.Subscribers[i].ConvertFrom(ctx, s)
// ConvertFrom implements apis.Convertible
func (sink *SubscribableTypeSpec) ConvertFrom(ctx context.Context, obj apis.Convertible) error {
switch source := obj.(type) {
case *duckv1beta1.SubscribableSpec:
if len(source.Subscribers) > 0 {
sink.Subscribable = &Subscribable{
Subscribers: make([]SubscriberSpec, len(source.Subscribers)),
}
for i, s := range source.Subscribers {
sink.Subscribable.Subscribers[i].ConvertFrom(ctx, s)
}
}
default:
Copy link
Copy Markdown
Contributor

@slinkydeveloper slinkydeveloper Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need the conversion to v1 too here?

Copy link
Copy Markdown
Member Author

@aliok aliok Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@slinkydeveloper slinkydeveloper Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok ok

return fmt.Errorf("unknown version, got: %T", source)
}
return nil
}

// ConvertFrom helps implement apis.Convertible
Expand Down
12 changes: 12 additions & 0 deletions pkg/apis/duck/v1alpha1/subscribable_types_conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,18 @@ func TestSubscribableTypeConversionWithV1Beta1(t *testing.T) {
}
}

func TestSubscribableTypeSpecStatusConversionBadType(t *testing.T) {
good, bad := &SubscribableTypeSpec{}, &SubscribableTypeSpec{}

if err := good.ConvertTo(context.Background(), bad); err == nil {
t.Errorf("ConvertTo() = %#v, wanted error", bad)
}

if err := good.ConvertFrom(context.Background(), bad); err == nil {
t.Errorf("ConvertFrom() = %#v, wanted error", good)
}
}

func TestSubscribableTypeStatusConversionBadType(t *testing.T) {
good, bad := &SubscribableTypeStatus{}, &SubscribableTypeStatus{}

Expand Down
1 change: 1 addition & 0 deletions pkg/apis/duck/v1beta1/subscribable_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ var (
_ apis.Listable = (*Subscribable)(nil)

_ apis.Convertible = (*Subscribable)(nil)
_ apis.Convertible = (*SubscribableSpec)(nil)
_ apis.Convertible = (*SubscribableStatus)(nil)
)

Expand Down
36 changes: 23 additions & 13 deletions pkg/apis/duck/v1beta1/subscribable_types_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,19 @@ func (source *Subscribable) ConvertTo(ctx context.Context, to apis.Convertible)
}

// ConvertTo helps implement apis.Convertible
func (source *SubscribableSpec) ConvertTo(ctx context.Context, sink *eventingduckv1.SubscribableSpec) {
if len(source.Subscribers) > 0 {
sink.Subscribers = make([]eventingduckv1.SubscriberSpec, len(source.Subscribers))
for i, s := range source.Subscribers {
s.ConvertTo(ctx, &sink.Subscribers[i])
func (source *SubscribableSpec) ConvertTo(ctx context.Context, obj apis.Convertible) error {
switch sink := obj.(type) {
case *eventingduckv1.SubscribableSpec:
if len(source.Subscribers) > 0 {
sink.Subscribers = make([]eventingduckv1.SubscriberSpec, len(source.Subscribers))
for i, s := range source.Subscribers {
s.ConvertTo(ctx, &sink.Subscribers[i])
}
}
default:
return fmt.Errorf("unknown version, got: %T", sink)
}

return nil
}

// ConvertTo helps implement apis.Convertible
Expand Down Expand Up @@ -91,21 +96,26 @@ func (sink *Subscribable) ConvertFrom(ctx context.Context, from apis.Convertible
case *eventingduckv1.Subscribable:
sink.ObjectMeta = source.ObjectMeta
sink.Status.ConvertFrom(ctx, &source.Status)
return sink.Spec.ConvertFrom(ctx, source.Spec)
return sink.Spec.ConvertFrom(ctx, &source.Spec)
default:
return fmt.Errorf("unknown version, got: %T", source)
}
}

// ConvertFrom helps implement apis.Convertible
func (sink *SubscribableSpec) ConvertFrom(ctx context.Context, source eventingduckv1.SubscribableSpec) error {
if len(source.Subscribers) > 0 {
sink.Subscribers = make([]SubscriberSpec, len(source.Subscribers))
for i, s := range source.Subscribers {
if err := sink.Subscribers[i].ConvertFrom(ctx, s); err != nil {
return err
func (sink *SubscribableSpec) ConvertFrom(ctx context.Context, obj apis.Convertible) error {
switch source := obj.(type) {
case *eventingduckv1.SubscribableSpec:
if len(source.Subscribers) > 0 {
sink.Subscribers = make([]SubscriberSpec, len(source.Subscribers))
for i, s := range source.Subscribers {
if err := sink.Subscribers[i].ConvertFrom(ctx, s); err != nil {
return err
}
}
}
default:
return fmt.Errorf("unknown version, got: %T", source)
}
return nil
}
Expand Down
12 changes: 12 additions & 0 deletions pkg/apis/duck/v1beta1/subscribable_types_conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,18 @@ func TestSubscribableTypeConversionWithV1(t *testing.T) {
}
}

func TestSubscribableSpecConversionBadType(t *testing.T) {
good, bad := &SubscribableSpec{}, &SubscribableSpec{}

if err := good.ConvertTo(context.Background(), bad); err == nil {
t.Errorf("ConvertTo() = %#v, wanted error", bad)
}

if err := good.ConvertFrom(context.Background(), bad); err == nil {
t.Errorf("ConvertFrom() = %#v, wanted error", good)
}
}

func TestSubscribableStatusConversionBadType(t *testing.T) {
good, bad := &SubscribableStatus{}, &SubscribableStatus{}

Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/messaging/v1beta1/channel_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func (sink *ChannelSpec) ConvertFrom(ctx context.Context, source v1.ChannelSpec)
sink.Delivery = &v1beta1.DeliverySpec{}
sink.Delivery.ConvertFrom(ctx, source.Delivery)
}
sink.ChannelableSpec.SubscribableSpec.ConvertFrom(ctx, source.ChannelableSpec.SubscribableSpec)
sink.ChannelableSpec.SubscribableSpec.ConvertFrom(ctx, &source.ChannelableSpec.SubscribableSpec)
}

// ConvertFrom helps implement apis.Convertible
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/messaging/v1beta1/in_memory_channel_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (sink *InMemoryChannelSpec) ConvertFrom(ctx context.Context, source v1.InMe
}
}
sink.SubscribableSpec = eventingduckv1beta1.SubscribableSpec{}
sink.SubscribableSpec.ConvertFrom(ctx, source.SubscribableSpec)
sink.SubscribableSpec.ConvertFrom(ctx, &source.SubscribableSpec)
return nil
}

Expand Down