Skip to content
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

Subscription v1<>v1beta conversion #3421

Merged
merged 3 commits into from
Jun 26, 2020
Merged

Subscription v1<>v1beta conversion #3421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c396fdf
Subscription v1<>v1beta conversion and simple tests
aliok Jun 26, 2020
b462de7
Formatting stuff
aliok Jun 26, 2020
37fa48b
Subscription v1<>v1beta conversion roundtrip tests
aliok Jun 26, 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
60 changes: 54 additions & 6 deletions pkg/apis/messaging/v1beta1/subscription_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,63 @@ import (
"context"
"fmt"

duckv1 "knative.dev/eventing/pkg/apis/duck/v1"
duckv1beta1 "knative.dev/eventing/pkg/apis/duck/v1beta1"

v1 "knative.dev/eventing/pkg/apis/messaging/v1"
Copy link
Member

Choose a reason for hiding this comment

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

messagingv1

"knative.dev/pkg/apis"
)

// ConvertTo implements apis.Convertible
func (source *Subscription) ConvertTo(ctx context.Context, sink apis.Convertible) error {
return fmt.Errorf("v1beta1 is the highest known version, got: %T", sink)
// ConvertTo implements apis.Convertible.
// Converts source (from v1beta1.Subscription) into v1.Subscription
func (source *Subscription) ConvertTo(ctx context.Context, obj apis.Convertible) error {
switch sink := obj.(type) {
case *v1.Subscription:
sink.ObjectMeta = source.ObjectMeta
sink.Spec.Channel = source.Spec.Channel
if source.Spec.Delivery != nil {
sink.Spec.Delivery = &duckv1.DeliverySpec{}
if err := source.Spec.Delivery.ConvertTo(ctx, sink.Spec.Delivery); err != nil {
return err
}
}
sink.Spec.Subscriber = source.Spec.Subscriber
sink.Spec.Reply = source.Spec.Reply

sink.Status.Status = source.Status.Status
sink.Status.PhysicalSubscription.SubscriberURI = source.Status.PhysicalSubscription.SubscriberURI
sink.Status.PhysicalSubscription.ReplyURI = source.Status.PhysicalSubscription.ReplyURI
sink.Status.PhysicalSubscription.DeadLetterSinkURI = source.Status.PhysicalSubscription.DeadLetterSinkURI
return nil
default:
return fmt.Errorf("Unknown conversion, got: %T", sink)

}
}

// ConvertFrom implements apis.Convertible
func (sink *Subscription) ConvertFrom(ctx context.Context, source apis.Convertible) error {
return fmt.Errorf("v1beta1 is the highest known version, got: %T", source)
// ConvertFrom implements apis.Convertible.
// Converts obj from v1.Subscription into v1beta1.Subscription
func (sink *Subscription) ConvertFrom(ctx context.Context, obj apis.Convertible) error {
switch source := obj.(type) {
case *v1.Subscription:
sink.ObjectMeta = source.ObjectMeta
sink.Spec.Channel = source.Spec.Channel
if source.Spec.Delivery != nil {
sink.Spec.Delivery = &duckv1beta1.DeliverySpec{}
if err := sink.Spec.Delivery.ConvertFrom(ctx, source.Spec.Delivery); err != nil {
return err
}
}
sink.Spec.Subscriber = source.Spec.Subscriber
sink.Spec.Reply = source.Spec.Reply

sink.Status.Status = source.Status.Status
sink.Status.PhysicalSubscription.SubscriberURI = source.Status.PhysicalSubscription.SubscriberURI
sink.Status.PhysicalSubscription.ReplyURI = source.Status.PhysicalSubscription.ReplyURI
sink.Status.PhysicalSubscription.DeadLetterSinkURI = source.Status.PhysicalSubscription.DeadLetterSinkURI

return nil
default:
return fmt.Errorf("Unknown conversion, got: %T", source)
}
}
232 changes: 232 additions & 0 deletions pkg/apis/messaging/v1beta1/subscription_conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,31 @@ package v1beta1
import (
"context"
"testing"

"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"
eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1"
duckv1beta1 "knative.dev/eventing/pkg/apis/duck/v1beta1"
v1 "knative.dev/eventing/pkg/apis/messaging/v1"
Copy link
Member

Choose a reason for hiding this comment

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

messagingv1

Copy link
Member

Choose a reason for hiding this comment

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

or remove the renaming

Copy link
Member Author

Choose a reason for hiding this comment

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

If you don't feel too strong about it, let's leave it as v1.
This test is about converting subscriptions from v1beta1 to v1. Name v1 is more understandable IMO as we're already in messaging v1beta1 package.

Copy link
Member Author

@aliok aliok Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

matmoor's bot suggested to add v1, so I added the name :)

Copy link
Member

Choose a reason for hiding this comment

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

Agree, but then we don't need to rename it, right?

Copy link
Member

Choose a reason for hiding this comment

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

Now, I saw a similar pattern for the other conversion and there we don't include messaging* or eventing*, sorry for the noise :)

"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
)

func TestSubscriptionConversionBadType(t *testing.T) {
good, bad := &Subscription{}, &Channel{}

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 TestBrokerConversionBadVersion(t *testing.T) {
good, bad := &Subscription{}, &Subscription{}

if err := good.ConvertTo(context.Background(), bad); err == nil {
Expand All @@ -32,3 +54,213 @@ func TestSubscriptionConversionBadType(t *testing.T) {
t.Errorf("ConvertFrom() = %#v, wanted error", good)
}
}

// Test v1beta1 -> v1 -> v1beta1
func TestSubscriptionConversionRoundTripV1beta1(t *testing.T) {
// Just one for now, just adding the for loop for ease of future changes.
versions := []apis.Convertible{&v1.Subscription{}}

linear := duckv1beta1.BackoffPolicyLinear

tests := []struct {
name string
in *Subscription
}{{
name: "min configuration",
in: &Subscription{
ObjectMeta: metav1.ObjectMeta{
Name: "subscription-name",
Namespace: "subscription-ns",
Generation: 17,
},
Spec: SubscriptionSpec{},
},
}, {
name: "full configuration",
in: &Subscription{
ObjectMeta: metav1.ObjectMeta{
Name: "subscription-name",
Namespace: "subscription-ns",
Generation: 17,
},
Spec: SubscriptionSpec{
Channel: corev1.ObjectReference{
Kind: "channelKind",
Namespace: "channelNamespace",
Name: "channelName",
APIVersion: "channelAPIVersion",
},
Subscriber: &duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "subscriber-dest-kind",
Namespace: "subscriber-dest-ns",
Name: "subscriber-dest-name",
APIVersion: "subscriber-dest-version",
},
URI: apis.HTTP("address"),
},
Reply: &duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "reply-dest-kind",
Namespace: "reply-dest-ns",
Name: "reply-dest-name",
APIVersion: "reply-dest-version",
},
URI: apis.HTTP("address"),
},
Delivery: &duckv1beta1.DeliverySpec{
DeadLetterSink: &duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "dlKind",
Namespace: "dlNamespace",
Name: "dlName",
APIVersion: "dlAPIVersion",
},
URI: apis.HTTP("dls"),
},
Retry: pointer.Int32Ptr(5),
BackoffPolicy: &linear,
BackoffDelay: pointer.StringPtr("5s"),
},
},
Status: SubscriptionStatus{
Status: duckv1.Status{
ObservedGeneration: 1,
Conditions: duckv1.Conditions{{
Type: "Ready",
Status: "True",
}},
},
PhysicalSubscription: SubscriptionStatusPhysicalSubscription{
SubscriberURI: apis.HTTP("subscriber.example.com"),
ReplyURI: apis.HTTP("reply.example.com"),
DeadLetterSinkURI: apis.HTTP("dlc.example.com"),
},
},
},
}}

for _, test := range tests {
for _, version := range versions {
t.Run(test.name, func(t *testing.T) {
ver := version
if err := test.in.ConvertTo(context.Background(), ver); err != nil {
t.Errorf("ConvertTo() = %v", err)
}
got := &Subscription{}
if err := got.ConvertFrom(context.Background(), ver); err != nil {
t.Errorf("ConvertFrom() = %v", err)
}

if diff := cmp.Diff(test.in, got); diff != "" {
t.Errorf("roundtrip (-want, +got) = %v", diff)
}
})
}
}
}

// Test v1 -> v1beta1 -> v1
func TestBrokerConversionRoundTripV1(t *testing.T) {
// Just one for now, just adding the for loop for ease of future changes.
versions := []apis.Convertible{&Subscription{}}

linear := eventingduckv1.BackoffPolicyLinear

tests := []struct {
name string
in *v1.Subscription
}{{
name: "min configuration",
in: &v1.Subscription{
ObjectMeta: metav1.ObjectMeta{
Name: "subscription-name",
Namespace: "subscription-ns",
Generation: 17,
},
Spec: v1.SubscriptionSpec{},
},
}, {
name: "full configuration",
in: &v1.Subscription{
ObjectMeta: metav1.ObjectMeta{
Name: "subscription-name",
Namespace: "subscription-ns",
Generation: 17,
},
Spec: v1.SubscriptionSpec{
Channel: corev1.ObjectReference{
Kind: "channelKind",
Namespace: "channelNamespace",
Name: "channelName",
APIVersion: "channelAPIVersion",
},
Subscriber: &duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "subscriber-dest-kind",
Namespace: "subscriber-dest-ns",
Name: "subscriber-dest-name",
APIVersion: "subscriber-dest-version",
},
URI: apis.HTTP("address"),
},
Reply: &duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "reply-dest-kind",
Namespace: "reply-dest-ns",
Name: "reply-dest-name",
APIVersion: "reply-dest-version",
},
URI: apis.HTTP("address"),
},
Delivery: &eventingduckv1.DeliverySpec{
DeadLetterSink: &duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "dlKind",
Namespace: "dlNamespace",
Name: "dlName",
APIVersion: "dlAPIVersion",
},
URI: apis.HTTP("dls"),
},
Retry: pointer.Int32Ptr(5),
BackoffPolicy: &linear,
BackoffDelay: pointer.StringPtr("5s"),
},
},
Status: v1.SubscriptionStatus{
Status: duckv1.Status{
ObservedGeneration: 1,
Conditions: duckv1.Conditions{{
Type: "Ready",
Status: "True",
}},
},
PhysicalSubscription: v1.SubscriptionStatusPhysicalSubscription{
SubscriberURI: apis.HTTP("subscriber.example.com"),
ReplyURI: apis.HTTP("reply.example.com"),
DeadLetterSinkURI: apis.HTTP("dlc.example.com"),
},
},
},
}}

for _, test := range tests {
for _, version := range versions {
t.Run(test.name, func(t *testing.T) {
ver := version
if err := ver.ConvertFrom(context.Background(), test.in); err != nil {
t.Errorf("ConvertFrom() = %v", err)
}
got := &v1.Subscription{}
if err := ver.ConvertTo(context.Background(), got); err != nil {
t.Errorf("ConvertTo() = %v", err)
}

if diff := cmp.Diff(test.in, got); diff != "" {
t.Errorf("roundtrip (-want, +got) = %v", diff)
}
})
}
}
}