Add support for cloudevent function signatures #29

roffjulie · 2020-06-03T17:45:53Z

Fixes #27

funcframework/framework.go

tbpg · 2020-06-03T18:30:57Z

funcframework/framework.go

+		log.Fatalf("failed to create protocol: %s", err.Error())
+	}
+
+	handleFn, err := cloudevents.NewHTTPReceiveHandler(context.Background(), p, fn)


What is this context used for? Should we pass it as an argument?

Possibly cancellation? I'm not positive. Adding it would make the function signature func(context.Context, cloudevents.Event). Is that acceptable? @grant

I think if this is something expected by cloudevents, we should pass it through. It would probably come from r.Context() at some point.

@grant do you have an opinion on the function signature? I'm inclined to agree with Tyler, despite the departure from the contract (particularly given that the .NET framework has as similar departure, with additional parameters that are language-idiomatic).

Yes this makes sense (ctx, ce).

Example function:
https://knative.dev/docs/eventing/samples/helloworld/helloworld-go/#recreating-the-sample-code

it is used for shutdown and lifecycle management of the process.

we also use it to pass in options to overload special things.

I don't know what ctx you're referring to here, Tyler, but if you're referring to why use context.Background(), it's because there is not any other context to use at this point in the code.

It feels like we should plumb a context.Context through as an argument from the user instead of using context.Background().

(Weird -- this was still a pending comment from a couple days ago, even through other comments went through. Sorry!)

funcframework/framework.go

funcframework/framework_test.go

tbpg

(Marking as request changes.)

README.md

funcframework/framework.go

grant

Marking as request changes per comments above.

…acy event

grant

1 nit. LGTM after that.

README.md

majelbstoat · 2020-06-09T00:02:08Z

FWIW, I'm not sure this will support cloud events as sent by gcp pubsub push, which doesn't send cloudevents data as headers, but as Attributes in the payload.

see cloudevents/sdk-go#306 (comment) for an example of what pubsub sends, and a later comment for how i'm currently working around it. You might reasonably call that an upstream issue within cloudevents/sdk-go, (or perhaps this will work and i just haven't grokked it) but i thought i'd mention it as i would consider GCP pubsub a primary use-case of this support.

grant · 2020-06-09T00:47:54Z

Are you using Cloud Functions @majelbstoat? Or Cloud Run? Or how exactly are you receiving Pub/Sub push? (Create an issue rather than PR comment?)

This PR is tangential to non CloudEvent payloads.

majelbstoat · 2020-06-09T00:54:37Z

I did open an issue, you closed it today ;) #26

This was using google cloud functions, but i also tried with cloud run. If you use the standard cloudevents sdk and the pubsub protocol, and then create a push subscription (which is what gcp documentation says to do for cloud functions/cloud run) you will get data back from pubsub in the format i described above.

(Might just be a mismatch of expectations, but if i emit cloudevents from the standard sdk, and i'm using a library to receive that says it unmarshals cloudevents, i expect to be able to receive structured cloud events, regardless of the transport. it feels like kind of the point of cloudevents. and especially as it's all GCP services.)

grant · 2020-06-09T01:30:34Z

I'd rather talk in an issue than a tangential PR comment.

Issue around supporting CEs (#26) was a duplicate of #27 which talks about supporting CEs more.

No public GCP product delivers CEs right now. I can check what samples might be wrong in a GitHub issue.

You can sign up for an alpha of delivering CEs from Pub/Sub to Run here:
https://forms.gle/ZhQoHYXo7ZFLWTRV6

roffjulie · 2020-06-09T17:12:48Z

ping @tbpg

funcframework/framework.go

tbpg · 2020-06-09T17:23:55Z

funcframework/framework.go

+		log.Fatalf("failed to create protocol: %s", err.Error())
+	}
+
+	handleFn, err := cloudevents.NewHTTPReceiveHandler(context.Background(), p, fn)


Use ctx here?

funcframework/framework.go

n3wscott · 2020-06-09T21:40:33Z

funcframework/framework.go

@@ -29,6 +29,7 @@ import (
 	"strings"

 	"cloud.google.com/go/functions/metadata"
+	cloudevents "github.com/cloudevents/sdk-go/v2"


:party-scott:

n3wscott · 2020-06-09T21:44:22Z

funcframework/framework.go

+	return nil
+}
+
+func registerCloudEventFunction(path string, fn func(context.Context, cloudevents.Event), h *http.ServeMux) error {


looks like this is coming from the sample: https://github.com/cloudevents/sdk-go/blob/master/samples/http/receiver-direct/main.go

It is loosely modeled on that sample, yes. Is that a problem?

grant · 2020-06-18T19:45:28Z

@tbpg Ping for review

funcframework/framework.go

Co-authored-by: Tyler Bui-Palsulich <[email protected]>

grant · 2020-06-22T15:47:39Z

We're approved @juliehockett. Let's merge.

michaelneale · 2020-06-24T02:30:26Z

since this was merged yesterday, pubsub functions on google functions with vendoring is failing: https://issuetracker.google.com/issues/159732630 could this be related?

…ures (GoogleCloudPlatform#29)" This reverts commit 1989552. The functions framework buildpack has a bug where it pulls from master (instead of from the latest pinned release) for vendored builds, and as this change is breaking, it prevents deployment. Fixes GoogleCloudPlatform#30, https://b.corp.google.com/issues/159732630

…ures (#29)" (#32) This reverts commit 1989552. The functions framework buildpack has a bug where it pulls from master (instead of from the latest pinned release) for vendored builds, and as this change is breaking, it prevents deployment. Fixes #30, https://b.corp.google.com/issues/159732630

Add support for cloudevent function signatures

5eb053b

roffjulie requested review from tbpg and grant June 3, 2020 17:46

Fix build error and bump cloud.google.go version

f8e69cb

tbpg suggested changes Jun 3, 2020

View reviewed changes

Addressing comments

a383b2c

roffjulie requested a review from tbpg June 4, 2020 17:57

tbpg suggested changes Jun 5, 2020

View reviewed changes

Julie Hockett added 3 commits June 8, 2020 07:22

Adding proper error handling to function registration

96c5bd1

remove autogenerated unecessary build file

1f8f735

Pass explicit function singatures for HTTP and cloudevents

91bed59

roffjulie requested a review from tbpg June 8, 2020 14:28

grant approved these changes Jun 8, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

grant reviewed Jun 8, 2020

View reviewed changes

funcframework/framework.go Outdated Show resolved Hide resolved

grant suggested changes Jun 8, 2020

View reviewed changes

Julie Hockett added 6 commits June 8, 2020 07:57

readme fixes

9bf5781

removing references to legacy cloud events andreplacing with just leg…

808baed

…acy event

readme indentation fix

ad9991b

readme indentation fix

2e37605

readme indentation fix

33a990c

readme indentation fix

7f684a1

roffjulie requested a review from grant June 8, 2020 15:25

grant approved these changes Jun 8, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

add ticks to readme

2f91f6c

grant mentioned this pull request Jun 8, 2020

Cannot unmarshal CloudEvent from pubsub push #26

Closed

tbpg suggested changes Jun 9, 2020

View reviewed changes

n3wscott reviewed Jun 9, 2020

View reviewed changes

Addressing comments:x

22cf840

roffjulie requested a review from tbpg June 10, 2020 20:11

Add context to cloudevent registration

5a0efc0

tbpg approved these changes Jun 22, 2020

View reviewed changes

funcframework/framework.go Outdated Show resolved Hide resolved

Apply suggested wording

1d0dfe3

Co-authored-by: Tyler Bui-Palsulich <[email protected]>

roffjulie merged commit 1989552 into GoogleCloudPlatform:master Jun 22, 2020

roffjulie deleted the cloudevents branch June 22, 2020 16:00

This was referenced Jun 23, 2020

Cloud Functions -> pubsub triggered function update leads to unexpected build error reffering to http triggered type hashicorp/terraform-provider-google#6662

Closed

Question -> cloud event support could possibly break the API during CF update #30

Closed

roffjulie mentioned this pull request Jun 24, 2020

Revert "Breaking change -- Add support for cloudevent function signat… #32

Merged

Add support for cloudevent function signatures #29

Add support for cloudevent function signatures #29

Uh oh!

Conversation

roffjulie commented Jun 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roffjulie Jun 3, 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!

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!

tbpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grant left a comment

Choose a reason for hiding this comment

Uh oh!

grant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

majelbstoat commented Jun 9, 2020

Uh oh!

grant commented Jun 9, 2020

Uh oh!

majelbstoat commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grant commented Jun 9, 2020

Uh oh!

roffjulie commented Jun 9, 2020

Uh oh!

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!

grant commented Jun 18, 2020

Uh oh!

Uh oh!

grant commented Jun 22, 2020

Uh oh!

michaelneale commented Jun 24, 2020

Uh oh!

Uh oh!

roffjulie Jun 3, 2020 •

edited

Loading

majelbstoat commented Jun 9, 2020 •

edited

Loading