Messaging v1beta1 Types by vaikas · Pull Request #2373 · knative/eventing

vaikas · 2020-01-10T21:03:31Z

Addresses #1869

Proposed Changes

Add v1beta1.[subscription,channel,inmemorychannel]
Add corresponding duckv1beta1 types

Release Note

NONE (missing reconcilers, etc. just the types).

knative-prow-robot · 2020-01-10T21:03:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas

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~~ [vaikas]

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

n3wscott · 2020-01-10T22:01:00Z

+	ReplyURI *apis.URL `json:"replyURI,omitempty"`
+
+	// ReplyURI is the fully resolved URI for the spec.delivery.deadLetterSink.
+	DeadLetterSinkURI *apis.URL `json:"deadLetterSinkURI,omitempty"`


the correct json should be deadLetterSinkUri`

Why is the channel error and this deadLetter?

vaikas · 2020-01-10T23:49:48Z

Thanks much! So far I think I addressed all the items that I could that I think we can do without some further input. Here's what I see the big ticket items that need resolving. Pulling them here
so they don't get lost in the comments:

Can Subscription specify DLC
- Yes as per @lionelvillard
Should DeliverySpec either be:

Moved to v1beta1 ✅

Change Subscription.Status.PhysicalSubscription to ResolvedSubscription

AND: rename json field: physicalSubscription -> resolved

What does (ties in with 1 above) Status expose.

vaikas · 2020-01-13T23:56:22Z

/test pull-knative-eventing-integration-tests

lionelvillard · 2020-01-16T19:13:35Z

+	// DeadLetterChannel is the reference to the native, platform specific channel
+	// where failed events are sent to.
+	// +optional
+	DeadLetterChannel *corev1.ObjectReference `json:"deadLetterChannel,omitempty"`


maybe the type should be api.URL.

In the status, it should be a reference to a Channel (well, really, Subscribable). That way you can Subscribe to the events that have failed. Make sense?

I had the same thought until I realized that this really is a ref to a channel that the owning channel created. I am +1 on DeadLetterChannel

Maybe this object also needs a deadlettersinkuri? If the resource has a delivery spec in the spec there is currently no place to show a resolved delivery deadleattersink.

Or there are two kinds of status objects, one where the channel makes you a deadletter channel, and ones that use the resolved deadletter destination

n3wscott · 2020-01-17T19:07:55Z

+// Channelable is a skeleton type wrapping Subscribable and Addressable in the manner we expect resource writers
+// defining compatible resources to embed it. We will typically use this type to deserialize
+// Channelable ObjectReferences and access their subscription and address data.  This is not a real resource.
+type Channelable struct {


If Delivery moved into Subscribable, then we can drop Channelable all together and a channel becomes

Addressable + Subscribable.

What do you think?

make sense to me. Big change though....

Besides removing one duck type (which I kind of think adds clarity), are there other benefits to removing it? I think being able to operate on a Channel ducktype as a single object in the consuming code base is kind of nice (instead of operating on Addressable / Subscribable) separately.
Not against it, just not sure what the concrete benefits of it are.

sorry, I meant that actually having the Channelable as a separate duck type adds clarity, not removing it. So, what are the benefits for removing it?

Simplify the number of things, channelable does not add anything new, just the union of two other ducks

But the code using it now needs to set up two watches if it cares about channels (one for addressable and one for subscribable) and extra serializations / etc.? Am I just confused here? Seems like it has downsides and I'm not seeing the upside of removing the duck.

Given the discussion above about Delivery NOT moving to Subscribable (since it needs to be available per Subscription) but moving to SubscriberSpec and the fact that there's also a DeadLetterChannel in the Channel, I don't see this happening. But perhaps I'm missing something here? So it's not only Subscribable, Addressable but also has DeadLetterChannel in the status.

knative-metrics-robot · 2020-01-18T04:05:14Z

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
pkg/apis/duck/v1beta1/channelable_types.go	Do not exist	87.5%
pkg/apis/duck/v1beta1/subscribable_types.go	Do not exist	100.0%
pkg/apis/messaging/v1beta1/channel_defaults.go	Do not exist	100.0%
pkg/apis/messaging/v1beta1/channel_lifecycle.go	Do not exist	83.3%
pkg/apis/messaging/v1beta1/channel_validation.go	Do not exist	96.2%
pkg/apis/messaging/v1beta1/in_memory_channel_defaults.go	Do not exist	0.0%
pkg/apis/messaging/v1beta1/in_memory_channel_lifecycle.go	Do not exist	75.0%
pkg/apis/messaging/v1beta1/in_memory_channel_validation.go	Do not exist	100.0%
pkg/apis/messaging/v1beta1/register.go	Do not exist	0.0%
pkg/apis/messaging/v1beta1/subscribable_channelable_validation.go	Do not exist	100.0%
pkg/apis/messaging/v1beta1/subscription_defaults.go	Do not exist	100.0%
pkg/apis/messaging/v1beta1/subscription_lifecycle.go	Do not exist	61.5%
pkg/apis/messaging/v1beta1/subscription_types.go	Do not exist	50.0%
pkg/apis/messaging/v1beta1/subscription_validation.go	Do not exist	93.3%

knative-prow-robot · 2020-01-18T04:05:24Z

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

Test name	Commit	Details	Rerun command
pull-knative-eventing-go-coverage	`589f5f5`	link	`/test pull-knative-eventing-go-coverage`

Full PR test history. Your PR dashboard.

Details

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.

n3wscott · 2020-01-21T19:41:02Z

+	// SubscribableStatus is the part where SubscribableStatus object is
 	// configured as to be compatible with Subscribable contract.
-	Status SubscribableTypeStatus `json:"status"`
+	Status SubscribableStatus `json:"status"`


I still want to question status. Is it really just a quick way to know spec.subscriber[x].generation == status.subscriber[x].generation?

If so, I think it should be removed.

If not, and it is the status of the subscriber, it should be removed because it is not the job of the channel to look up the subscribers.

If it is the status of the data plane result (like, we are getting 2xx's) I also think it should be renamed or rethought because if this is false, it is useless without a reason and message.

I was thinking it would be that the subscription has been wired up. You asked for a subscription in the Spec, reconciler then goes and makes that, how would you know if something went wrong? But, I'm reading some of the more of the codes now.

Then maybe you are pro-"add a reason and a message"?

status.subscriber[x].ready: false falls short of being useful info to act on.

Ack. @n3wscott and I talked offline. So the spec.Array and status.Array has precedent in things like pod (container in spec -> containerstatuses). so using array in spec and array in status to match part is all good. I'll create a more targeted issue for possibly renaming / modifying the fields within the SubscriberStatus struct.

I created this issue to track this in specific. As we move forward with the implementation of the controller and channels, etc. we will revisit this as part of that issue:
#2416

n3wscott · 2020-01-21T19:42:06Z

 // GetListType implements apis.Listable
-func (c *SubscribableType) GetListType() runtime.Object {
-	return &SubscribableTypeList{}
+func (c *Subscribable) GetListType() runtime.Object {


this reads so much better, nice work.

n3wscott · 2020-01-21T23:27:39Z

/lgtm

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 Jan 10, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 10, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 10, 2020

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

knative-prow-robot requested review from matzew and steuhs January 10, 2020 21:03

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Jan 10, 2020

n3wscott reviewed Jan 10, 2020

View reviewed changes

vaikas force-pushed the messaging-v1beta1 branch 2 times, most recently from f78c785 to 58f704c Compare January 13, 2020 20:12

vaikas force-pushed the messaging-v1beta1 branch from 9bec93a to fbc0e24 Compare January 15, 2020 02:11

matzew reviewed Jan 15, 2020

View reviewed changes

Comment thread pkg/apis/duck/v1beta1/subscribable_types.go

matzew reviewed Jan 15, 2020

View reviewed changes

Comment thread pkg/apis/duck/v1beta1/subscribable_types.go Outdated

matzew reviewed Jan 15, 2020

View reviewed changes

Comment thread pkg/apis/duck/v1beta1/subscribable_types.go

matzew reviewed Jan 15, 2020

View reviewed changes

Comment thread pkg/apis/messaging/v1beta1/subscription_types.go

lionelvillard reviewed Jan 16, 2020

View reviewed changes

Comment thread pkg/apis/duck/v1beta1/delivery_types.go

lionelvillard reviewed Jan 16, 2020

View reviewed changes

Comment thread pkg/apis/duck/v1beta1/subscribable_types.go Outdated

lionelvillard reviewed Jan 16, 2020

View reviewed changes

Comment thread pkg/apis/duck/v1beta1/subscribable_types.go Outdated

This was referenced Jan 17, 2020

correcting JSON to camelCase for Uri... #2400

Closed

Minior updates on 'correcting' JSON to camelCase... knative/eventing-contrib#835

Closed

mattmoor reviewed Jan 17, 2020

View reviewed changes

n3wscott reviewed Jan 17, 2020

View reviewed changes

Comment thread pkg/apis/duck/v1beta1/channelable_types.go

n3wscott reviewed Jan 17, 2020

View reviewed changes

Comment thread pkg/apis/messaging/v1beta1/channel_types.go Outdated

n3wscott reviewed Jan 17, 2020

View reviewed changes

Comment thread pkg/apis/messaging/v1beta1/channel_types.go Outdated

n3wscott reviewed Jan 17, 2020

View reviewed changes

Comment thread pkg/apis/messaging/v1beta1/channel_types.go Outdated

vaikas added 14 commits January 17, 2020 19:59

add v1beta1 for subscription

216802b

channel_types + codegen

42394aa

inmemory channel

ab21cc6

get ducks in a row

42e174c

boilerplate realized it is a new year indeed

e52d7ac

address PR comments

af21a67

hoist deliverytypes to v1beta1 and update accordingly

06300b1

also imc to v1beta1 ducks

7ef5f00

rollback some vendored files that break things

309e6f2

rollback bunch of accidental vendor upgrades

8d9c398

more nonsense...

415ffbb

clean up based on PR comments and rethinking more. add tests

bdfb2d1

copyright year

4a509f1

Simply internal structures, make more consistent

589f5f5

vaikas force-pushed the messaging-v1beta1 branch from 40d0d25 to 589f5f5 Compare January 18, 2020 04:01

vaikas mentioned this pull request Jan 18, 2020

Flows v1beta1 #2407

Merged

n3wscott reviewed Jan 21, 2020

View reviewed changes

Comment thread pkg/apis/messaging/v1beta1/channel_types.go

n3wscott reviewed Jan 21, 2020

View reviewed changes

Comment thread pkg/apis/messaging/v1beta1/in_memory_channel_validation.go

vaikas mentioned this pull request Jan 21, 2020

Figure out the appropriate fields in v1beta1 SubscribableStatus. #2416

Closed

vaikas changed the title ~~WIP: Messaging v1beta1 Types~~ Messaging v1beta1 Types Jan 21, 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 Jan 21, 2020

knative-prow-robot assigned n3wscott Jan 21, 2020

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

knative-prow-robot merged commit e4533ef into knative:master Jan 21, 2020

vaikas deleted the messaging-v1beta1 branch January 21, 2020 23:51

Conversation

vaikas commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Jan 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaikas commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaikas commented Jan 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Jan 18, 2020

Uh oh!

knative-prow-robot commented Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

n3wscott commented Jan 21, 2020

Uh oh!

Reviewers

Assignees

Labels

vaikas commented Jan 10, 2020 •

edited

Loading

vaikas commented Jan 10, 2020 •

edited

Loading

knative-prow-robot commented Jan 18, 2020 •

edited

Loading