Migrate Subscription reconciler to use messaging.* v1 resources by nlopezgi · Pull Request #3930 · knative/eventing

nlopezgi · 2020-08-25T16:41:14Z

Part of #3584

Proposed Changes

Migrate Subscription reconciler to use messaging.* v1 resources

Removing tests that deal with v1alpha1 subscription (according to #3789 (comment) - see also #3871)

nlopezgi · 2020-08-25T17:36:03Z

fyi this is ready for review @aliok

aliok · 2020-08-26T06:58:36Z

/assign

aliok · 2020-08-26T07:00:39Z

 var (
-	v1beta1ChannelGVK = v1beta1.SchemeGroupVersion.WithKind("Channel")
-	v1ChannelGVK      = v1.SchemeGroupVersion.WithKind("Channel")
+	//v1ChannelGVK = v1.SchemeGroupVersion.WithKind("Channel")


thanks for catching, I had it staged but forgot to push that change cleaning up

aliok · 2020-08-26T07:04:30Z

 }

-func (r *Reconciler) getSubStatusV1Beta1(subscription *v1beta1.Subscription, channel *eventingduckv1alpha1.ChannelableCombined) (eventingduckv1beta1.SubscriberStatus, error) {
+func (r *Reconciler) getSubStatusV1Beta1(subscription *v1.Subscription, channel *eventingduckv1alpha1.ChannelableCombined) (eventingduckv1.SubscriberStatus, error) {


Could you rename this function to ...V1?

aliok · 2020-08-26T07:10:30Z

/approve

Some nits.
Other than those all good.

matzew · 2020-08-26T13:02:46Z

 }

-func (r *Reconciler) getSubStatusV1Alpha1(subscription *v1beta1.Subscription, channel *eventingduckv1alpha1.ChannelableCombined) (eventingduckv1beta1.SubscriberStatus, error) {
+func (r *Reconciler) getSubStatusV1Alpha1(ctx context.Context, subscription *v1.Subscription, channel *eventingduckv1alpha1.ChannelableCombined) (eventingduckv1.SubscriberStatus, error) {


perhaps also a "bad" name here ...

..... (eventingduckv1.SubscriberStatus, error)

this actually is getting the status from the v1Alpha1 channel passed and returning a v1 status. I think the current name works here, but let me know if you have concrete suggestions. Thanks!

I think the name here is good :)

nlopezgi

thanks for the review. fixed the nits.

nlopezgi · 2020-08-26T13:30:59Z

 }

-func (r *Reconciler) getSubStatusV1Beta1(subscription *v1beta1.Subscription, channel *eventingduckv1alpha1.ChannelableCombined) (eventingduckv1beta1.SubscriberStatus, error) {
+func (r *Reconciler) getSubStatusV1Beta1(subscription *v1.Subscription, channel *eventingduckv1alpha1.ChannelableCombined) (eventingduckv1.SubscriberStatus, error) {


nlopezgi · 2020-08-26T13:31:24Z

 var (
-	v1beta1ChannelGVK = v1beta1.SchemeGroupVersion.WithKind("Channel")
-	v1ChannelGVK      = v1.SchemeGroupVersion.WithKind("Channel")
+	//v1ChannelGVK = v1.SchemeGroupVersion.WithKind("Channel")


thanks for catching, I had it staged but forgot to push that change cleaning up

nlopezgi · 2020-08-26T13:33:16Z

 }

-func (r *Reconciler) getSubStatusV1Alpha1(subscription *v1beta1.Subscription, channel *eventingduckv1alpha1.ChannelableCombined) (eventingduckv1beta1.SubscriberStatus, error) {
+func (r *Reconciler) getSubStatusV1Alpha1(ctx context.Context, subscription *v1.Subscription, channel *eventingduckv1alpha1.ChannelableCombined) (eventingduckv1.SubscriberStatus, error) {


this actually is getting the status from the v1Alpha1 channel passed and returning a v1 status. I think the current name works here, but let me know if you have concrete suggestions. Thanks!

knative-metrics-robot · 2020-08-26T13:41:40Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/subscription/subscription.go	85.3%	76.3%	-9.0

aliok

/approve
/lgtm

Thanks!

nlopezgi · 2020-08-26T15:04:35Z

thanks @aliok !
fyi @matzew this needs a renewed approved label

matzew

/lgtm
/approve

knative-prow-robot · 2020-08-27T07:10:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, matzew, nlopezgi

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [matzew]

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

…ive#3930) * migrate subscription to v1 messaging * stash changes * review comments

Nicolas Lopez added 2 commits August 25, 2020 11:58

migrate subscription to v1 messaging

b9fdde8

stash changes

9816e90

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 added the cla: yes Indicates the PR's author has signed the CLA. label Aug 25, 2020

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 25, 2020

knative-prow-robot requested review from aliok and liu-cong August 25, 2020 16:41

nlopezgi changed the title ~~[WIP] - Migrate Subscription reconciler to use messaging.* v1 resources~~ Migrate Subscription reconciler to use messaging.* v1 resources Aug 25, 2020

knative-prow-robot removed 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

knative-prow-robot assigned aliok Aug 26, 2020

aliok reviewed Aug 26, 2020

View reviewed changes

matzew reviewed Aug 26, 2020

View reviewed changes

review comments

640382e

nlopezgi commented Aug 26, 2020

View reviewed changes

aliok approved these changes Aug 26, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2020

matzew approved these changes Aug 27, 2020

View reviewed changes

knative-prow-robot assigned matzew Aug 27, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2020

knative-prow-robot merged commit 58c0cd7 into knative:master Aug 27, 2020

matzew pushed a commit to matzew/eventing that referenced this pull request Aug 27, 2020

Migrate Subscription reconciler to use messaging.* v1 resources (knat…

9326a01

…ive#3930) * migrate subscription to v1 messaging * stash changes * review comments

aliok mentioned this pull request Sep 25, 2020

Add aliok to approvers #4118

Merged

Conversation

nlopezgi commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

nlopezgi commented Aug 25, 2020

Uh oh!

aliok commented Aug 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliok commented Aug 26, 2020

Uh oh!

matzew Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlopezgi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Aug 26, 2020

Uh oh!

aliok left a comment

Choose a reason for hiding this comment

Uh oh!

nlopezgi commented Aug 26, 2020

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nlopezgi commented Aug 25, 2020 •

edited

Loading

matzew Aug 26, 2020 •

edited

Loading