WIP: change storage version to v1beta1 #1504
Conversation
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
knative-prow-robot
added
the
do-not-merge/work-in-progress
knative-prow-robot
added
the
size/S
|
[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 |
knative-prow-robot
added
the
approved
| @@ -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" | |||
There was a problem hiding this comment.
@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...
There was a problem hiding this comment.
This doesn't mean that this is the KafkaChannel version.
This is the duck type version KafkaChannel supports. So, it should be v1.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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] = "v1alpha1" | ||
| c.Annotations[messaging.SubscribableDuckVersionAnnotation] = "v1beta1" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, I'd like us to get rid of the v1alpha1 shapes asap.
|
/assign |
|
@matzew: The following test failed, say
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. |
|
@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. |
|
More specifically:
Update: we can have the migration and tooling for it (release script, adaption of the testing scripts) later. |
|
About required tooling changes: #1394 (comment) |
knative-prow-robot
added
the
needs-rebase
|
@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. |
|
/close Created tickets in new repo, |
|
@aliok: Closed this PR. In response to this:
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. |
Signed-off-by: Matthias Wessendorf mwessend@redhat.com
Fixes #
Proposed Changes
v1beta1Release Note
Docs