Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

WIP: change storage version to v1beta1 #1504

Closed
wants to merge 1 commit into from
Closed

WIP: change storage version to v1beta1 #1504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions kafka/channel/config/300-kafka-channel.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ spec:
versions:
- name: v1alpha1
served: true
storage: true
storage: false
- name: v1beta1
served: true
storage: false
storage: true
conversion:
strategy: Webhook
conversionReviewVersions: ["v1beta1", "v1alpha1"]
Expand Down
2 changes: 1 addition & 1 deletion kafka/channel/pkg/apis/messaging/v1alpha1/kafka_channel_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (c *KafkaChannel) SetDefaults(ctx context.Context) {
c.Annotations = make(map[string]string)
}
if _, ok := c.Annotations[messaging.SubscribableDuckVersionAnnotation]; !ok {
c.Annotations[messaging.SubscribableDuckVersionAnnotation] = "v1alpha1"
c.Annotations[messaging.SubscribableDuckVersionAnnotation] = "v1beta1"
Copy link
Member

Choose a reason for hiding this comment

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

I am not so sure about this change here.
v1alpha1 Channelable duck type support is removed from eventing.

I think we should get rid of the the v1alpha1 channel resource completely, providing a migration script.

cc @vaikas for confirmation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'd like us to get rid of the v1alpha1 shapes asap.

}

c.Spec.SetDefaults(ctx)
Expand Down
6 changes: 3 additions & 3 deletions kafka/channel/pkg/apis/messaging/v1alpha1/kafka_channel_defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestKafkaChannelDefaults(t *testing.T) {
initial: KafkaChannel{},
expected: KafkaChannel{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1alpha1"},
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1beta1"},
},
Spec: KafkaChannelSpec{
NumPartitions: utils.DefaultNumPartitions,
Expand All @@ -54,7 +54,7 @@ func TestKafkaChannelDefaults(t *testing.T) {
},
expected: KafkaChannel{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1alpha1"},
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1beta1"},
},
Spec: KafkaChannelSpec{
NumPartitions: utils.DefaultNumPartitions,
Expand All @@ -70,7 +70,7 @@ func TestKafkaChannelDefaults(t *testing.T) {
},
expected: KafkaChannel{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1alpha1"},
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1beta1"},
},
Spec: KafkaChannelSpec{
NumPartitions: testNumPartitions,
Expand Down
2 changes: 1 addition & 1 deletion kafka/channel/pkg/apis/messaging/v1beta1/kafka_channel_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (c *KafkaChannel) SetDefaults(ctx context.Context) {
c.Annotations = make(map[string]string)
}
if _, ok := c.Annotations[messaging.SubscribableDuckVersionAnnotation]; !ok {
c.Annotations[messaging.SubscribableDuckVersionAnnotation] = "v1"
Copy link
Member Author

Choose a reason for hiding this comment

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

@aliok why was this at v1? 🤔

I also checked here: https://github.com/knative/eventing/pull/3169/files and there (matching to IMC) it was at beta1...

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't mean that this is the KafkaChannel version.

This is the duck type version KafkaChannel supports. So, it should be v1.

Copy link
Member

Choose a reason for hiding this comment

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

I also checked here: https://github.com/knative/eventing/pull/3169/files and there (matching to IMC) it was at beta1...

It should've been v1 too. But, there's no difference between v1beta1 and v1 Channelable/Subscribable duck type, so, v1beta1 should also work. ...until v1beta1 support is deleted.

Copy link
Member

Choose a reason for hiding this comment

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

So, we should revert this to v1

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so we have to default the annotation to the version that we are currently storing. Why? Because for different serving versions we run the conversion hook, and we can adjust the annotation to match if we fiddle with the fields. For things that we are storing, however, we can't do any modifications because the conversion hook does not run. As @aliok says, since they are the same we should be safe here, but just wanted to give the reasoning for why we set the annotations differently and what the "correct" way to do that is. Hope that helps @matzew @aliok

c.Annotations[messaging.SubscribableDuckVersionAnnotation] = "v1beta1"
}

c.Spec.SetDefaults(ctx)
Expand Down
6 changes: 3 additions & 3 deletions kafka/channel/pkg/apis/messaging/v1beta1/kafka_channel_defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestKafkaChannelDefaults(t *testing.T) {
initial: KafkaChannel{},
expected: KafkaChannel{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1"},
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1beta1"},
},
Spec: KafkaChannelSpec{
NumPartitions: utils.DefaultNumPartitions,
Expand All @@ -54,7 +54,7 @@ func TestKafkaChannelDefaults(t *testing.T) {
},
expected: KafkaChannel{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1"},
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1beta1"},
},
Spec: KafkaChannelSpec{
NumPartitions: utils.DefaultNumPartitions,
Expand All @@ -70,7 +70,7 @@ func TestKafkaChannelDefaults(t *testing.T) {
},
expected: KafkaChannel{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1"},
Annotations: map[string]string{"messaging.knative.dev/subscribable": "v1beta1"},
},
Spec: KafkaChannelSpec{
NumPartitions: testNumPartitions,
Expand Down