Flow controller by vaikas · Pull Request #189 · knative/eventing

vaikas · 2018-07-13T19:46:20Z

Addresses #178

Proposed Changes

Add Flow Controller that can target objects which implement Status.ServiceName
For now the example targets Revision directly since Route / Service do not yet expose it
Flow Controller resolves the action target and creates a channel and subscription, feed and wires them all together.

google-prow-robot · 2018-07-13T19:46:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas-google

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-google]

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

vaikas · 2018-07-13T20:16:11Z

/assign @scothis
/assign @mattmoor

markfisher · 2018-07-13T20:42:03Z

@@ -0,0 +1,57 @@
+# Flow Example
+
+The flow sample includes a function that listes for k8s events on a cluster and a flow


typo: s/listes/listens/

markfisher · 2018-07-13T20:45:53Z


 ```
-ko apply -f config/buses/stub.yaml
+ko apply -f config/buses/stub-bus.yaml


actually: ko apply -f config/buses/stub/

(or: ko apply -f config/buses/stub/stub-bus.yaml)

markfisher · 2018-07-13T20:49:44Z

+	return newFinalizers, nil
+}
+
+func newFloweNonControllerRef(et *v1alpha1.Flow) *metav1.OwnerReference {


typo in function name ("Flowe")

Is this used? Also, shouldn't we use proper controller owner references?

scothis

looks good so far, nits inline

scothis · 2018-07-13T20:35:40Z

+# Once Route supports serviceName, use that
+#      kind: Route
+      kind: Revision
+      apiVersion: serving.knative.dev/v1alpha1


I wish we didn't have to include apiVersion here, but Service vs Service makes that impractical.

It's not only that, but also we need to allow other people to add Revision (or Service or whatever) that's not hardcoded. I suppose we could have 'known types' short hands notation, but I don't really like that approach.

scothis · 2018-07-13T20:36:22Z


 ```
-ko apply -f config/buses/stub.yaml
+ko apply -f config/buses/stub-bus.yaml


config/buses/stub/stub-bus.yaml

scothis · 2018-07-13T20:44:00Z

+First, install the Stub bus, if not already installed. You **must** change the Kind to ClusterBus
+
+```
+ko apply -f config/buses/stub-bus.yaml


config/buses/stub/stub-bus.yaml

yep, @markfisher also pointed this out, done.

there were two, this one still needs to be updated

oops, sorry. my bad. fixed.

scothis · 2018-07-13T20:45:15Z

+
+## Deploy
+
+First, install the Stub bus, if not already installed. You **must** change the Kind to ClusterBus


I assume this is a temporary restriction 😄

scothis · 2018-07-13T20:59:00Z

+		return c.resolveObjectReference(namespace, action.Target)
+	}
+	if action.TargetURI != nil {
+		return *action.TargetURI, nil


There's an impedance mismatch between a uri and a service name. A service name is equivalent to a hostname.

Yeah, I understood from Wednesday call that we wanted a URI and not just a hostname.

we will need to deal with it where it bottoms out (Subscription in this case), recognizing whether there's a scheme or not and handling accordingly

...OR we could create an ExternalName-typed Service here and then use its name?

I think my preference would be to flow it through to as is and let the lower levels deal with it. At least for now.

scothis · 2018-07-13T21:08:26Z

+}
+
+// AddFinalizer adds value to the list of finalizers on obj
+func AddFinalizer(obj runtimetypes.Object, value string) error {


Where is the finalizer used?

scothis · 2018-07-13T21:11:05Z

+	return newFinalizers, nil
+}
+
+func newFloweNonControllerRef(et *v1alpha1.Flow) *metav1.OwnerReference {


Is this used? Also, shouldn't we use proper controller owner references?

scothis · 2018-07-13T21:19:31Z

+			Subscriber: target,
+		},
+	}
+	return c.clientset.ChannelsV1alpha1().Subscriptions(flow.Namespace).Create(subscription)


add an owner reflrence to cascade deletes

scothis · 2018-07-13T21:19:57Z

+	if flow.Spec.Trigger.ParametersFrom != nil {
+		feed.Spec.Trigger.ParametersFrom = flow.Spec.Trigger.ParametersFrom
+	}
+


add owner reference

vaikas · 2018-07-13T22:08:42Z

Done, PTAL.

scothis · 2018-07-13T22:54:53Z

/lgtm

markfisher · 2018-07-13T22:55:03Z

/lgtm

Flows support a TargetURI as an escape hatch instead of a Target resource. This value is passed through to the Subscription's subscriber. Buses will now accept an http:// or https:// based URI as the subscriber. It is generally recomended to avoid using a URI unless nessesary. A DNS host name is preferred as it will be more compatible with the future event delivery protocol, which will detect/negotiate the underlying transport. Refs #189

@evankanderson

@evankanderson

From [ROLES.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/ROLES.md#approver): > Reviewer of the codebase for at least 3 months or 50% of project lifetime, whichever is shorter - [First Issue](#80). Opened 6/11 - [First PR](#66). Opened 5/31 - [First Review](#79 (review)) 6/11 > Primary reviewer for at least 10 substantial PRs to the codebase - #422 (review) - #414 (review) - #325 (review) - #225 (review) - #189 (review) - #168 (review) - #165 (review) - #99 (review) - #79 (review) - #111 (review) > Reviewed or merged at least 30 PRs to the codebase - [Reviewed 23 PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+reviewed-by%3Ascothis) - [Authored 34 merged PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+author%3Ascothis+is%3Amerged) - [Authored 5 open PRs](https://github.com/knative/eventing/pulls/scothis) > Nominated by an area lead From [WORKING_GROUPS.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/WORKING-GROUPS.md#events) /assign @vaikas-google > With no objections from other leads 🤞 /cc @evankanderson @grantr @inlined @mattmoor

@evankanderson

Ville Aikas added 6 commits July 13, 2018 18:42

WIP: Controller for flow

1501c3a

checkpoint before rebasing

c8fbb53

Add flows to codegen. Result autogenerated code

39a95f9

forgot to add register.go

6443637

Wire in channel / subscriptions reconcile

db85de1

e2e happy path works now

fccd7be

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2018

google-prow-robot requested review from evankanderson and ultrasaurus July 13, 2018 19:46

google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 13, 2018

google-prow-robot assigned mattmoor and scothis Jul 13, 2018

markfisher reviewed Jul 13, 2018

View reviewed changes

Address pr comments

c8ed0fd

scothis reviewed Jul 13, 2018

View reviewed changes

add ownerrefs

be69b3d

fix link to stub bus

0582e45

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2018

google-prow-robot assigned markfisher Jul 13, 2018

google-prow-robot merged commit d982c95 into knative:master Jul 13, 2018

scothis mentioned this pull request Jul 17, 2018

Handle subscriber URIs #214

Merged

scothis mentioned this pull request Sep 11, 2018

Add scothis as an approver #426

Merged

		@@ -0,0 +1,57 @@
		# Flow Example

		The flow sample includes a function that listes for k8s events on a cluster and a flow


		## Deploy

		First, install the Stub bus, if not already installed. You must change the Kind to ClusterBus

Conversation

vaikas commented Jul 13, 2018

Proposed Changes

Uh oh!

google-prow-robot commented Jul 13, 2018

Uh oh!

vaikas commented Jul 13, 2018

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!

scothis 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!

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!

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!

vaikas commented Jul 13, 2018

Uh oh!

scothis commented Jul 13, 2018

Uh oh!

markfisher commented Jul 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants