-
Notifications
You must be signed in to change notification settings - Fork 117
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
Conversation
…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
FYI @cumason123 |
There was a problem hiding this 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 allcloudevent
parsing logic from theevent
signature.
- I don't think the
There was a problem hiding this 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.
There was a problem hiding this 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!
…ons DO NOT return a custom value. General tidy-up. Support binary functions.
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
There was a problem hiding this 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.
examples/README.md
Outdated
|
||
|
||
|
There was a problem hiding this comment.
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.
src/functions_framework/__init__.py
Outdated
class _EventType(enum.Enum): | ||
LEGACY = 1 | ||
CLOUDEVENT_BINARY = 2 | ||
CLOUDEVENT_TEXT = 3 |
There was a problem hiding this comment.
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
.
src/functions_framework/__init__.py
Outdated
function(event) | ||
|
||
|
||
def _run_text_cloudevent(function, request, cloudevent_def): |
There was a problem hiding this comment.
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
.
There was a problem hiding this 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.
There was a problem hiding this 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!
Cool, looks like we're all good/approved. :) |
* 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 <[email protected]> * Update tests/test_cloudevent_functions.py Co-authored-by: Dustin Ingram <[email protected]> * Update tests/test_cloudevent_functions.py Co-authored-by: Dustin Ingram <[email protected]> * Update src/functions_framework/__init__.py Co-authored-by: Dustin Ingram <[email protected]> * Update tests/test_functions/cloudevents/main.py Co-authored-by: Dustin Ingram <[email protected]> * 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 <[email protected]> * Update examples/cloudevents/README.md Co-authored-by: Dustin Ingram <[email protected]> * Update examples/cloudevents/README.md Co-authored-by: Dustin Ingram <[email protected]> * Update examples/cloudevents/main.py Co-authored-by: Dustin Ingram <[email protected]> * Update tests/test_view_functions.py Co-authored-by: Dustin Ingram <[email protected]> * 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 <[email protected]> * Fix enum per reviewer suggestion. * Rename text event => strucuture event. Co-authored-by: Joel Gerard <[email protected]> Co-authored-by: Dustin Ingram <[email protected]> Co-authored-by: joelgerard <[email protected]> Co-authored-by: Joel Gerard <[email protected]>
* 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 <[email protected]>
* 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 <[email protected]> * reverted auto format Signed-off-by: Curtis Mason <[email protected]> * lint fixes Signed-off-by: Curtis Mason <[email protected]> * changed cloudevents to <=1.0 in setup Signed-off-by: Curtis Mason <[email protected]> * made cloudevents==1.0 Signed-off-by: Curtis Mason <[email protected]> * added cloudevent_view tests Signed-off-by: Curtis Mason <[email protected]> * test lint fixes Signed-off-by: Curtis Mason <[email protected]> * implemented try_catch in cloudevent view wrapper Signed-off-by: Curtis Mason <[email protected]> * import fix Signed-off-by: Curtis Mason <[email protected]> * adjusted cloud_run_cloudevents readme Signed-off-by: Curtis Mason <[email protected]> * added elif for cloudevent Signed-off-by: Curtis Mason <[email protected]> * adjusted README Signed-off-by: Curtis Mason <[email protected]> * upgraded to cloudevents 1.0.1 Signed-off-by: Curtis Mason <[email protected]> * import ordering lint fix Signed-off-by: Curtis Mason <[email protected]> * removed event from readme cloudevents section Signed-off-by: Curtis Mason <[email protected]> * resolved various nits and reverted event code Signed-off-by: Curtis Mason <[email protected]> * dockerfile env variables Signed-off-by: Curtis Mason <[email protected]> * Update examples/cloud_run_cloudevents/main.py Co-authored-by: Dustin Ingram <[email protected]> Signed-off-by: Curtis Mason <[email protected]> * cleaned up test_cloudevent_functions Signed-off-by: Curtis Mason <[email protected]> * Update examples/cloud_run_cloudevents/Dockerfile Co-authored-by: Adam Ross <[email protected]> Signed-off-by: Curtis Mason <[email protected]> * tunneled cloud_exceptions in flask abort Signed-off-by: Curtis Mason <[email protected]> * Added additional documentation in sample code Signed-off-by: Curtis Mason <[email protected]> * added time to tests Signed-off-by: Curtis Mason <[email protected]> * Update README.md Co-authored-by: Dustin Ingram <[email protected]> * Update README.md Co-authored-by: Dustin Ingram <[email protected]> * Update README.md Co-authored-by: Dustin Ingram <[email protected]> * Update README.md Co-authored-by: Dustin Ingram <[email protected]> * Update examples/cloud_run_cloudevents/send_cloudevent.py Co-authored-by: Dustin Ingram <[email protected]> * Update examples/cloud_run_cloudevents/README.md Co-authored-by: Dustin Ingram <[email protected]> * Update examples/cloud_run_cloudevents/README.md Co-authored-by: Dustin Ingram <[email protected]> * Update src/functions_framework/__init__.py Co-authored-by: Dustin Ingram <[email protected]> * Update src/functions_framework/__init__.py Co-authored-by: Dustin Ingram <[email protected]> * cloudevents 1.1.0 update Signed-off-by: Curtis Mason <[email protected]> * simplified exceptions debug Signed-off-by: Curtis Mason <[email protected]> * simplified cloudevent view test Signed-off-by: Curtis Mason <[email protected]> * Update src/functions_framework/__init__.py Co-authored-by: Dustin Ingram <[email protected]> * shebang cloudevent executable Signed-off-by: Curtis Mason <[email protected]> * cloudevents version bump Signed-off-by: Curtis Mason <[email protected]> * Removed InvalidStructuredJSON exception Signed-off-by: Curtis Mason <[email protected]> * Don't bump version in a feature branch * Add back missing CHANGELOG lines * Reformat with latest black Co-authored-by: Dustin Ingram <[email protected]> Co-authored-by: Katie McLaughlin <[email protected]> Co-authored-by: Adam Ross <[email protected]>
Adds support for Cloud Events per #55.