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

wants to merge 1 commit into from

Conversation

matzew
Copy link
Member

@matzew matzew commented Aug 25, 2020

Signed-off-by: Matthias Wessendorf mwessend@redhat.com

Fixes #

Proposed Changes

  • setting storage version to v1beta1

Release Note

Docs

@matzew
change storage version to v1beta1
dc07fcc 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 25, 2020
@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 25, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2020
matzew
matzew commented Aug 25, 2020
View reviewed changes
kafka/channel/pkg/apis/messaging/v1beta1/kafka_channel_defaults.go
@@ -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

aliok
aliok reviewed Aug 25, 2020
View reviewed changes
kafka/channel/pkg/apis/messaging/v1alpha1/kafka_channel_defaults.go
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.

@aliok
Copy link
Member

aliok commented Aug 25, 2020

/assign

@aliok
Copy link
Member

aliok commented Aug 25, 2020

@matzew #1396 is the ticket for this PR.
feel free to assign it to yourself.

@knative-prow-robot
Copy link
Contributor

@matzew: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-eventing-contrib-integration-tests dc07fcc link /test pull-knative-eventing-contrib-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@aliok
Copy link
Member

aliok commented Aug 25, 2020

@matzew there's also the source and we need to do the same thing there too.

Let me create the setup for migration and then we can do the same thing for channel as well.

@aliok
Copy link
Member

aliok commented Aug 25, 2020 •
edited
Loading

More specifically:

Update: we can have the migration and tooling for it (release script, adaption of the testing scripts) later.
Let's try to merge this one.

@aliok
Copy link
Member

aliok commented Aug 27, 2020

About required tooling changes: #1394 (comment)

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2020
@knative-prow-robot
Copy link
Contributor

@matzew: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This was referenced Sep 30, 2020
Closed
Closed
Closed
@aliok
Copy link
Member

aliok commented Sep 30, 2020

/close

Created tickets in new repo, knative-sandbox/eventing-kafka for the remaining tasks: knative-extensions/eventing-kafka#65

@knative-prow-robot
Copy link
Contributor

@aliok: Closed this PR.

In response to this:

/close

Created tickets in new repo, knative-sandbox/eventing-kafka for the remaining tasks: knative-extensions/eventing-kafka#65

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This was referenced Sep 30, 2020
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aliok aliok aliok left review comments

@lberk lberk Awaiting requested review from lberk

@vaikas vaikas Awaiting requested review from vaikas

Assignees

@aliok aliok

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matzew @knative-prow-robot @aliok @vaikas @googlebot