Cloud events 0.1 by inlined · Pull Request #53 · knative/eventing

inlined · 2018-04-26T21:33:45Z

Fixes Issue #47

Proposed Changes

Implements the 0.1 ratified version of CloudEvents' transport on HTTP.

Known bugs:

Non-ASCII characters for context properties in binary encoding is not yet supported
Extension property names are case-lossy in binary encoding (spec bug; raised with CloudEvents group)

vaikas · 2018-04-27T23:54:01Z

+
+package event
+
+// BUG(inlined): must add header encoding/decoding


Nit: can we make this into a more traditional: TODO(inlined)?

vaikas · 2018-04-27T23:56:45Z

-		return nil, err
-	}
+func isJSONEncoding(encoding string) bool {
+	return encoding == contentTypeJSON || encoding == "text/json"


Above there's binary json too so curious if this catches them all or if that's the intention?

I'll add a comment. The spec is a bit annoying that the envelope and the data are orthogonally encoded. This is meant to just check the valid data encodings regardless of the outer encoding (structured JSON, binary JSON, some new unknown format, etc)

vaikas · 2018-04-27T23:56:57Z

+
+package event
+
+// BUG(inlined): must add header encoding/decoding


Nit: can we make this into a more traditional: TODO(inlined)?

vaikas · 2018-04-27T23:57:48Z

+		return nil, err
+	}
+
+	if err := assertRequiredFields(context); err != nil {


nit, assert to me implies 'assert' action as in things fail noisily. consider renaming this to verifyRequiredFields.

vaikas · 2018-04-28T00:10:08Z

+	if h.dataType.Kind() == reflect.Ptr {
+		elemType = h.dataType.Elem()
+	}
+	dataPtrVal := reflect.New(elemType)


There's a lot of reflection going on here, I think it would be nice to have some comments on what's going on there.

vaikas · 2018-04-28T00:10:46Z

+
+// Mux allows developers to handle logically related groups of
+// functionality multiplexed based on the event type.
+// BUG: Mux relies on JSON encoding for events.


instead of BUG, consider TODO?

inlined

Rebased against master since we're choosing to abandon changes this was diffed against.

Going to work on a follow up that makes this expose @evankanderson's proposed API signature. I'd also like to eventually be smarter about the encoding (e.g. can the mime package help?) and ideally also support Google's dialect so we can promote this to its own repo & use it within GCF and Knative.

inlined · 2018-05-30T10:42:31Z

-		return nil, err
-	}
+func isJSONEncoding(encoding string) bool {
+	return encoding == contentTypeJSON || encoding == "text/json"


I'll add a comment. The spec is a bit annoying that the envelope and the data are orthogonally encoded. This is meant to just check the valid data encodings regardless of the outer encoding (structured JSON, binary JSON, some new unknown format, etc)

inlined · 2018-06-06T15:54:39Z

+
+package event
+
+// BUG(inlined): must add header encoding/decoding


inlined · 2018-06-06T15:55:24Z

+		return nil, err
+	}
+
+	if err := assertRequiredFields(context); err != nil {


vaikas · 2018-06-06T21:34:05Z

@@ -0,0 +1,16 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")


No need for bazel build files anymore, now use 'ko'.

vaikas · 2018-06-06T21:35:19Z

+	ctx.CloudEventsVersion = "0.1"
+	ctx.EventTime = time.Now().UTC()
+
+	if ctx.EventID == "" {


Maybe just use this package instead:
https://github.com/google/uuid

vaikas · 2018-06-06T21:37:24Z

 	"testing"
 	"time"

 	"github.com/elafros/eventing/pkg/event"


Looks like this file might not have been updated yet? should be github.com/knative/eventing/pkg/event?

googlebot · 2018-07-11T16:48:32Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

inlined · 2018-07-11T16:48:42Z

The diff is too large; I'm going to create a new local branch, cherry pick affected files, and force push a single commit with the relevant diffs

googlebot · 2018-07-11T17:05:46Z

CLAs look good, thanks!

google-prow-robot · 2018-07-11T17:05:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inlined

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

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

vaikas

mainly just some nits, but please change the copyright to match the other files.

vaikas · 2018-07-11T17:27:37Z

@@ -0,0 +1,99 @@
+/*
+Copyright 2018 Google LLC


Can you change this to match the other headers (Knative authors...)

vaikas · 2018-07-11T20:41:35Z

+*/
+
+// Implements a simple utility for sending a JSON-encoded sample event.
+// Not wired to bazel because this utility is meant to be run directly rather


Not sure what the comment about bazel is. Regardless we don't use bazel anymore.

vaikas · 2018-07-11T20:42:22Z

+)
+
+func init() {
+	flag.StringVar(&context.EventID, "event-id", "", "Event ID to use. Defaults to a UUID")


Nit: Defaults to a generated UUID.
In case somebody takes it as a specific string UUID :)

vaikas · 2018-07-11T20:46:38Z

+		h.Set(name, value)
+	}
+}
+func getReqHeader(h http.Header, name string, value *string) error {


nit, I would prefer getRequiredHeader so it's not confused with something like getRequestHeader.

vaikas · 2018-07-11T20:48:23Z

+	return &e.Context, nil
+}
+
+// NewRequest craetes an HTTP request for Structured content encoding.


nit: creates

vaikas · 2018-07-11T20:54:51Z

+	}
+}
+
+// Verifies the that a function has the right number of in and out params and that they are


s/the that/that/

vaikas · 2018-07-11T20:58:21Z

+
+// Handler creates an EventHandler that implements http.Handler
+// Will panic in case of a type error
+// @param fn a function of type func(<your data struct>, *event.Context) error


I don't think @param does anything here? Perhaps:

fn a function of type func(...

Done; will change again in the next iteration to add more signatures.

vaikas · 2018-07-11T20:59:50Z

+// (error)
+func assertOutParamSignature(fnType reflect.Type) {
+	if fnType.NumOut() != 1 {
+		panic(usage + "; wrong output count")


Can you print here something like:
; wrong output count. Expected 1 got %d", fnType.NumOut()

vaikas · 2018-07-11T21:05:34Z

+	return fnType.In(0)
+}
+
+// Alocates a new instance of type t and returns:


inlined · 2018-07-12T00:54:33Z

/retest
It seems like I got a spurious permission error?

google-prow-robot · 2018-07-12T00:56:28Z

@inlined: The following test failed, say /retest to rerun them all:

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

vaikas · 2018-07-12T14:15:34Z

/lgtm

Includes the following changes 1e7c2e7 Update static_watcher.go d688349 Include note about embedding the ManualWatcher in the InformedWatcher 767d6a5 Few changes to the configmap package 0122abd The test logger will now log the correct caller (knative#63) e7a4b0d Dont call flag.parse in pkg/test (knative#62) b213523 Update test-infra dependency (knative#61) 382a2bf Make kube_checks generic so that it can be used in serving (knative#58) 760afb6 cleanup (knative#57) eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54) 6eff182 Remove docker repo from e2e flags (knative#53) 4be5c07 Vendor the test-infra scripts (knative#52) 8c687df Update WaitForEndpointState to return response (knative#51) 8f6a3be Update knative/pkg/test (knative#50) 3ca4270 Add a logkey for the reconcile key. (knative#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43) f4a77d7 Add a common test clients file (knative#46) 450739d Add common test logging module (knative#45)

Includes the following changes: 8fc80de Few changes to the configmap package (knative#59) 0122abd The test logger will now log the correct caller (knative#63) e7a4b0d Dont call flag.parse in pkg/test (knative#62) b213523 Update test-infra dependency (knative#61) 382a2bf Make kube_checks generic so that it can be used in serving (knative#58) 760afb6 cleanup (knative#57) eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54) 6eff182 Remove docker repo from e2e flags (knative#53) 4be5c07 Vendor the test-infra scripts (knative#52) 8c687df Update WaitForEndpointState to return response (knative#51) 8f6a3be Update knative/pkg/test (knative#50) 3ca4270 Add a logkey for the reconcile key. (knative#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43) f4a77d7 Add a common test clients file (knative#46) 450739d Add common test logging module (knative#45

* bump knative/pkg to 8fc80de Includes the following changes: 8fc80de Few changes to the configmap package (#59) 0122abd The test logger will now log the correct caller (#63) e7a4b0d Dont call flag.parse in pkg/test (#62) b213523 Update test-infra dependency (#61) 382a2bf Make kube_checks generic so that it can be used in serving (#58) 760afb6 cleanup (#57) eedc0a9 Make verify-codegen.sh compatible with OS X (#54) 6eff182 Remove docker repo from e2e flags (#53) 4be5c07 Vendor the test-infra scripts (#52) 8c687df Update WaitForEndpointState to return response (#51) 8f6a3be Update knative/pkg/test (#50) 3ca4270 Add a logkey for the reconcile key. (#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (#43) f4a77d7 Add a common test clients file (#46) 450739d Add common test logging module (#45 * Use updated methods that indicate that the configmap.Watcher is using an informer

vaikas reviewed Apr 28, 2018

View reviewed changes

evankanderson assigned vaikas May 3, 2018

inlined commented Jun 6, 2018

View reviewed changes

vaikas reviewed Jun 6, 2018

View reviewed changes

Cherry-pick relevant changes from ee3936c

49ef509

inlined changed the base branch from v0.1 to master July 11, 2018 17:00

PR comments

94d85b7

vaikas reviewed Jul 11, 2018

View reviewed changes

PR feedback; mostly fixing copyrights

9ac9c1a

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

google-prow-robot merged commit d64bbec into knative:master Jul 12, 2018

evankanderson mentioned this pull request Jul 12, 2018

Enable passing CloudEvents headers ('CE-*') #180

Closed

inlined mentioned this pull request Jul 13, 2018

Events utility is out of date #47

Closed


		package event

		// BUG(inlined): must add header encoding/decoding

		@@ -0,0 +1,16 @@
		load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")

Conversation

inlined commented Apr 26, 2018

Proposed Changes

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!

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

googlebot commented Jul 11, 2018

Uh oh!

inlined commented Jul 11, 2018

Uh oh!

googlebot commented Jul 11, 2018

Uh oh!

google-prow-robot commented Jul 11, 2018

Uh oh!

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

google-prow-robot commented Jul 12, 2018 •

edited

Loading