Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Cloud Events support for #55 #56

Merged
merged 50 commits into from
Jun 19, 2020

Conversation

joelgerard
Copy link
Contributor

@joelgerard joelgerard commented Jun 10, 2020

Adds support for Cloud Events per #55.

Joel Gerard and others added 5 commits June 5, 2020 17:07
…o be run from anywhere and enables IDE debugger
I will squash a bunch of these interim commits before submitting the PR.

DO NOT SUBMIT
…e error handling.

Please see all the TODO questions before I finish off this PR.

DO NOT SUBMIT
@joelgerard joelgerard requested review from di and grant June 10, 2020 18:45
@grant
Copy link
Contributor

grant commented Jun 10, 2020

FYI @cumason123

@grant grant requested a review from roffjulie June 10, 2020 21:36
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @joelgerard! I like the tests and think this is close to mergeable.
I added a detailed review.

There are a couple scenarios we're in that make it tricky, that we faced. Hope this will give background:

  • The CloudEvents SDK is pretty clunky and not ideal. It looks like we've been able to use it here though.
  • The legacy (event) signature (elif signature_type == "event":) and _event_view_func_wrapper is pretty wrong as GCF legacy events are not CloudEvents and are a separate payload type.
    • I don't think the _is_binary_cloud_event codepath is touched. We should remove all cloudevent parsing logic from the event signature.

tests/test_functions/events/main.py Outdated Show resolved Hide resolved
tests/test_functions/events/main.py Outdated Show resolved Hide resolved
tests/test_functions/events/main.py Outdated Show resolved Hide resolved
tests/test_functions.py Outdated Show resolved Hide resolved
tests/test_functions.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
Copy link

@roffjulie roffjulie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good starting point, but it misses one of the key points of the CloudEvent proposal, which is that the function signature containing a CloudEvent has a different signature type.

In brief, there are three types of function signatures: http, which is the function that handles an HTTP request directly, event, which is the function that accepts legacy events parsed by the framework into (data, context) parameters, and cloudevent, which is the function that accepts CloudEvents parsed by the framework into an event as defined by the CloudEvents SDK. See the framework contract (https://github.com/GoogleCloudPlatform/functions-framework#supported-function-types) for more depth, but that's the broad summary.

It'd be great if this could be reworked to introduce the cloudevent signature type as a separate type from the existing legacy event type.

tests/test_functions/events/main.py Outdated Show resolved Hide resolved
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @joelgerard!

.gitignore Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
tests/test_functions.py Outdated Show resolved Hide resolved
tests/test_functions/events/main.py Outdated Show resolved Hide resolved
tests/test_functions/events/main.py Outdated Show resolved Hide resolved
tests/test_functions/events/main.py Outdated Show resolved Hide resolved
tests/test_functions.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tests/test_cloudevent_functions.py Outdated Show resolved Hide resolved
tests/test_cloudevent_functions.py Outdated Show resolved Hide resolved
tests/test_cloudevent_functions.py Outdated Show resolved Hide resolved
tests/test_cloudevent_functions.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
tests/test_view_functions.py Show resolved Hide resolved
tests/test_functions/cloudevents/main.py Outdated Show resolved Hide resolved
tests/test_functions/cloudevents/main.py Outdated Show resolved Hide resolved
joelgerard and others added 10 commits June 15, 2020 16:56
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
src/functions_framework/__init__.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/cloudevents/README.md Outdated Show resolved Hide resolved
examples/cloudevents/README.md Outdated Show resolved Hide resolved
examples/cloudevents/README.md Outdated Show resolved Hide resolved
examples/cloudevents/main.py Outdated Show resolved Hide resolved
tests/test_functions/cloudevents/main.py Outdated Show resolved Hide resolved
tests/test_view_functions.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
examples/README.md Show resolved Hide resolved
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 non-critical requested changes.

Comment on lines 6 to 8



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Here and in the main README there >1 blank lines in the file. Remove these extra blank lines.

class _EventType(enum.Enum):
LEGACY = 1
CLOUDEVENT_BINARY = 2
CLOUDEVENT_TEXT = 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned before, CloudEvent Text is not an event type.

The logic used here is for a CloudEvent Structured event.
This is the spec: https://github.com/cloudevents/spec/blob/master/http-protocol-binding.md#32-structured-content-mode

CLOUDEVENT_TEXT should be CLOUDEVENT_STRUCTURED.

function(event)


def _run_text_cloudevent(function, request, cloudevent_def):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be _run_structured_cloudevent.

Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending @grant's changes.

Copy link

@roffjulie roffjulie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for seeing this through!

@grant
Copy link
Contributor

grant commented Jun 19, 2020

Cool, looks like we're all good/approved. :)

@grant grant merged commit eddbc7e into GoogleCloudPlatform:2.0.0-dev Jun 19, 2020
di added a commit that referenced this pull request Jun 23, 2020
* Ignore IDE files.

* Use the test file directory as a basis instead of cwd. Allows tests to be run from anywhere and enables IDE debugger

* Add support for Cloud Events. Rough draft.

I will squash a bunch of these interim commits before submitting the PR.

DO NOT SUBMIT

* Return the functions return value. Test Cloud Events SDK 0.3. Add some error handling.

Please see all the TODO questions before I finish off this PR.

DO NOT SUBMIT

* Minor cleanup. Split test code.

* Clean up unused paths, split large test files into two, ensure functions DO NOT return a custom value. General tidy-up. Support binary functions.

* Fix lint errors with black.

* Fix lint errors with black.

* Update setup.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update tests/test_cloudevent_functions.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update tests/test_cloudevent_functions.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update src/functions_framework/__init__.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update tests/test_functions/cloudevents/main.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Clearer imports.

* don't factor out routes.

* Add a TODO for testing the different combinations of events and signature types.

* Add cloudevent as a signature type in the argument list.

* Clarify import.

* Clarify import.

* A sample that shows how to use a CloudEvent.

* In the case of a sig type / event type mismatch throw a 400

* Update the docs to use CloudEvent sig type instead of Event sig type. Note that I wrote the "Event" type is deprecated. Not sure if this is accurate.

* Lint fixes.

* Tests for checking correct event type corresponds to correct function sig. Fixed abort import error.

* Sort imports.

* Remove old example.

* Readme to explain how to run the sample locally.

* Rename cloud_event to cloudevent

* For legacy docs, add a notice to the new docs.

* There is no 1.1 event type.

* use the term cloudevent rather than event everywhere where we are talking about a CloudEvent to disambiguate these signature types.

* Update examples/cloudevents/README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update examples/cloudevents/README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update examples/cloudevents/README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update examples/cloudevents/main.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update tests/test_view_functions.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Add legacy event back to docs.

* Add legacy event back to docs.

* Use abort from flask for consistency and fix return in event test.

* update docs and error messages to better mirror the other runtimes.

* Minor fixes to docs w.r.t. naming.

* Update src/functions_framework/__init__.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Fix enum per reviewer suggestion.

* Rename text event => strucuture event.

Co-authored-by: Joel Gerard <joelgerard@google.com>
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

Co-authored-by: joelgerard <joelgerard@users.noreply.github.com>
Co-authored-by: Joel Gerard <joelgerard@google.com>
di added a commit that referenced this pull request Jul 6, 2020
di added a commit that referenced this pull request Jul 6, 2020
* Revert "Version 2.0.0 (#67)"

This reverts commit f2471b4.

* Revert "Add Cloud Events support for #55 (#56) (#64)"

This reverts commit 8f3fe35.

* Version 1.5.0
di added a commit that referenced this pull request Aug 19, 2020
* Revert "Version 2.0.0 (#67)"

This reverts commit f2471b4.

* Revert "Add Cloud Events support for #55 (#56) (#64)"

This reverts commit 8f3fe35.

* Version 1.5.0
di added a commit that referenced this pull request Aug 19, 2020
* Version 1.5.0 (#69)

* Revert "Version 2.0.0 (#67)"

This reverts commit f2471b4.

* Revert "Add Cloud Events support for #55 (#56) (#64)"

This reverts commit 8f3fe35.

* Version 1.5.0

* Add legacy GCF Python 3.7 behavior (#77)

* Add legacy GCF Python 3.7 behavior

* Add test

* Modify tests

* Version 1.6.0 (#81)

Co-authored-by: Arjun Srinivasan <69502+asriniva@users.noreply.github.com>
di added a commit that referenced this pull request Oct 22, 2020
* Version 1.5.0 (#69)

* Revert "Version 2.0.0 (#67)"

This reverts commit f2471b4.

* Revert "Add Cloud Events support for #55 (#56) (#64)"

This reverts commit 8f3fe35.

* Version 1.5.0

* Improve documentation around Dockerfiles (#70)

* Add a link to an example Dockerfile in the top README.md

* Update the inline Dockerfile to match file

* Remove explicit gunicorn installation

* make readme links absolute, useful

Useful for when this readme appears on both github and pypi

* added cloudevents 1.0.0

Signed-off-by: Curtis Mason <cumason@google.com>

* reverted auto format

Signed-off-by: Curtis Mason <cumason@google.com>

* lint fixes

Signed-off-by: Curtis Mason <cumason@google.com>

* changed cloudevents to <=1.0 in setup

Signed-off-by: Curtis Mason <cumason@google.com>

* made cloudevents==1.0

Signed-off-by: Curtis Mason <cumason@google.com>

* added cloudevent_view tests

Signed-off-by: Curtis Mason <cumason@google.com>

* test lint fixes

Signed-off-by: Curtis Mason <cumason@google.com>

* implemented try_catch in cloudevent view wrapper

Signed-off-by: Curtis Mason <cumason@google.com>

* import fix

Signed-off-by: Curtis Mason <cumason@google.com>

* adjusted cloud_run_cloudevents readme

Signed-off-by: Curtis Mason <cumason@google.com>

* added elif for cloudevent

Signed-off-by: Curtis Mason <cumason@google.com>

* adjusted README

Signed-off-by: Curtis Mason <cumason@google.com>

* upgraded to cloudevents 1.0.1

Signed-off-by: Curtis Mason <cumason@google.com>

* import ordering lint fix

Signed-off-by: Curtis Mason <cumason@google.com>

* removed event from readme cloudevents section

Signed-off-by: Curtis Mason <cumason@google.com>

* resolved various nits and reverted event code

Signed-off-by: Curtis Mason <cumason@google.com>

* dockerfile env variables

Signed-off-by: Curtis Mason <cumason@google.com>

* Update examples/cloud_run_cloudevents/main.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
Signed-off-by: Curtis Mason <cumason@google.com>

* cleaned up test_cloudevent_functions

Signed-off-by: Curtis Mason <cumason@google.com>

* Update examples/cloud_run_cloudevents/Dockerfile

Co-authored-by: Adam Ross <grayside@gmail.com>
Signed-off-by: Curtis Mason <cumason@google.com>

* tunneled cloud_exceptions in flask abort

Signed-off-by: Curtis Mason <cumason@google.com>

* Added additional documentation in sample code

Signed-off-by: Curtis Mason <cumason@google.com>

* added time to tests

Signed-off-by: Curtis Mason <cumason@google.com>

* Update README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update examples/cloud_run_cloudevents/send_cloudevent.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update examples/cloud_run_cloudevents/README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update examples/cloud_run_cloudevents/README.md

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update src/functions_framework/__init__.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* Update src/functions_framework/__init__.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* cloudevents 1.1.0 update

Signed-off-by: Curtis Mason <cumason@google.com>

* simplified exceptions debug

Signed-off-by: Curtis Mason <cumason@google.com>

* simplified cloudevent view test

Signed-off-by: Curtis Mason <cumason@google.com>

* Update src/functions_framework/__init__.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>

* shebang cloudevent executable

Signed-off-by: Curtis Mason <cumason@google.com>

* cloudevents version bump

Signed-off-by: Curtis Mason <cumason@google.com>

* Removed InvalidStructuredJSON exception

Signed-off-by: Curtis Mason <cumason@google.com>

* Don't bump version in a feature branch

* Add back missing CHANGELOG lines

* Reformat with latest black

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
Co-authored-by: Katie McLaughlin <glasnt@google.com>
Co-authored-by: Adam Ross <grayside@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants