Flows v1beta1 by vaikas · Pull Request #2407 · knative/eventing

vaikas · 2020-01-18T04:23:45Z

Addresses #1869

Proposed Changes

Add v1beta1.[sequence,parallel]
Types only, nothing else

Release Note

Add v1beta1.[parallel,sequence] types only.

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

[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-21T20:47:56Z

+	duckv1.Status `json:",inline"`
+
+	// IngressChannelStatus corresponds to the ingress channel status.
+	IngressChannelStatus ParallelChannelStatus `json:"ingressChannelStatus"`


I would remove Status and have it just be obj.status.ingressChannel

I might even trim it all the way down to obj.status.ingress

I prefer to leave the statuses there. Just like in pods, in there's containerStatuses.

Furthermore, status can contain other fields where it's not really conveying Status of another resource, so in this particular case, I think having Status there adds clarity.

ah right k8s precedent. ok! 👌

n3wscott · 2020-01-21T20:54:00Z

+	ReadyCondition apis.Condition `json:"ready"`
+}
+
+type ParallelSubscriptionStatus struct {


I would combine ParallelChannelStatus and ParallelSubscriptionStatus to be the same object, maybe an array of conditions for subscriptions that related to the channel. But it kinda feels overkill because this data is all in the channel object already and will be propagated to its ready condition.

n3wscott · 2020-01-21T20:57:08Z

+
+	// Reply is a Reference to where the result of the last Subscriber gets sent to.
+	// +optional
+	Reply *duckv1.Destination `json:"reply,omitempty"`


why is this a separate thing? it feels like Reply could be deleted and the operator just adds this to the last step in steps.

Sorry, I don't understand this. If I have a sequence, I can either have it terminate or the result of the last sequence step gets wired to send it's output here. Separate from what?
As an example here:
https://knative.dev/docs/eventing/sequence/
You can have a sequence that's "terminal" and sequence with reply.

what is the diff between:

Steps: A->B->C->ReplyX

vs

Steps: A->B->C
Reply: ReplyX

is there an extra channel created for the first version?

Where would you specify ReplyX in the first case?

ah, I think I understand. Why can't the last step just decide? Ideally these would be reusable components so if you wire the last piece into the sequence, seemed more cumbersome to be able to reuse
the A->B->C
and then depending on your application be able to reuse only that part.

Reply is needed so that there is no ambiguity when composing/reusing sequences.

n3wscott · 2020-01-21T20:58:00Z

+	Channel corev1.ObjectReference `json:"channel"`
+
+	// ReadyCondition indicates whether the Channel is ready or not.
+	ReadyCondition apis.Condition `json:"ready"`


This needs a reason and message to be useful

Feels similar to issue with Subscription related issue?
#2416

Yeah same issue.

n3wscott · 2020-01-21T20:58:05Z

+	Subscription corev1.ObjectReference `json:"subscription"`
+
+	// ReadyCondition indicates whether the Subscription is ready or not.
+	ReadyCondition apis.Condition `json:"ready"`


This needs a reason and message to be useful

Feels similar to issue with Subscription related issue?
#2416

vaikas · 2020-01-22T02:10:55Z

Thanks @n3wscott PTAL.

knative-metrics-robot · 2020-01-23T19:51: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/apis/flows/v1beta1/parallel_defaults.go	Do not exist	0.0%
pkg/apis/flows/v1beta1/parallel_lifecycle.go	Do not exist	0.0%
pkg/apis/flows/v1beta1/parallel_validation.go	Do not exist	0.0%
pkg/apis/flows/v1beta1/register.go	Do not exist	0.0%
pkg/apis/flows/v1beta1/sequence_defaults.go	Do not exist	100.0%
pkg/apis/flows/v1beta1/sequence_lifecycle.go	Do not exist	2.3%
pkg/apis/flows/v1beta1/sequence_validation.go	Do not exist	0.0%
pkg/apis/messaging/v1beta1/subscription_lifecycle.go	61.5%	57.1%	-4.4

knative-prow-robot · 2020-01-23T19:52:00Z

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

Test name	Commit	Details	Rerun command
pull-knative-eventing-go-coverage	`83ffba1`	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.

lionelvillard · 2020-01-23T21:59:47Z

+type SequenceSpec struct {
+	// Steps is the list of Destinations (processors / functions) that will be called in the order
+	// provided.
+	Steps []duckv1.Destination `json:"steps"`


to discuss: #2302. My main argument is one of the reasons for retrying is because a service depends on a backend service that is known to be not very reliable. And one reason to limit retrying is because it might cost real money each time a call is made. That's why I'm arguing for fine-grained delivery control.

n3wscott · 2020-01-24T15:45:48Z

/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 18, 2020

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

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

mattmoor reviewed Jan 18, 2020

View reviewed changes

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release labels Jan 18, 2020

knative-prow-robot requested review from chizhg and evankanderson January 18, 2020 04:24

mattmoor reviewed Jan 18, 2020

View reviewed changes

n3wscott reviewed Jan 21, 2020

View reviewed changes

vaikas added 6 commits January 21, 2020 16:09

add parallel/sequence types

65fddd5

codegen for parallel/sequence

df77eea

moar parallel/sequence stuff

6cffb98

move methods to lifecycle where they belong

1fdf410

move to messaging v1beta1 now that it was merged

ba38e0e

address pr feedback

2e76445

vaikas force-pushed the flows-v1beta1 branch from 33b3619 to 2e76445 Compare January 22, 2020 02:10

n3wscott reviewed Jan 22, 2020

View reviewed changes

Comment thread pkg/apis/flows/v1beta1/parallel_types.go

mark ParallelBranch.Filter as optional

83ffba1

lionelvillard reviewed Jan 23, 2020

View reviewed changes

knative-prow-robot assigned n3wscott Jan 24, 2020

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

vaikas changed the title ~~[WIP] Flows v1beta1~~ Flows v1beta1 Jan 24, 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 24, 2020

knative-prow-robot merged commit e67a675 into knative:master Jan 24, 2020

vaikas deleted the flows-v1beta1 branch January 24, 2020 16:48

nachocano mentioned this pull request Mar 6, 2020

Create v1beta1 types google/knative-gcp#614

Closed

This was referenced Jun 17, 2020

Implement v1 API for flows.* resources #3335

Closed

Implement V1 API for messaging.* resources. #3336

Closed

Implement V1 API for eventing.* resources. #3337

Closed

nachocano mentioned this pull request Jun 26, 2020

Move ContainerSource to v1beta1 #3426

Closed

nachocano mentioned this pull request Jul 8, 2020

Move SinkBinding to v1beta1 #3544

Closed

8 tasks

Conversation

vaikas commented Jan 18, 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 18, 2020

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!

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!

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!

vaikas commented Jan 22, 2020

Uh oh!

Uh oh!

knative-metrics-robot commented Jan 23, 2020

Uh oh!

knative-prow-robot commented Jan 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Jan 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vaikas commented Jan 18, 2020 •

edited

Loading