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

Update the handler to match style of golang-http-template #16

Merged
merged 3 commits into from
Apr 4, 2019
Merged

Update the handler to match style of golang-http-template #16

merged 3 commits into from
Apr 4, 2019

Conversation

kturcios
Copy link
Contributor

@kturcios kturcios commented Feb 21, 2019

Description

  • Update the template to match the (event, context) style of the golang-http-template.
  • Add dev build options
  • Update README with instructions on how to use the template
  • Add waitress as the production server

Motivation and Context

This will allow users to return custom responses for their functions.

  • I have raised an issue to propose this change (required)

Which issue(s) this PR fixes

Fixes #8
Fixes #15
Resolves #12

How Has This Been Tested?

  • All examples in README were built and tested for expected responses for python3-http
  • All examples in README were built and tested for expected responses for python3-http-armhf

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: Kevin Turcios kevin_turcios@outlook.com

- Created two new templates, python3-http & python3-http-armhf
- Added waitress as the production server for new templates

Signed-off-by: Kevin Turcios <kevin_turcios@outlook.com>
$ curl -i $OPENFAAS_URL/function/<fn-name>
```

## Event and Context Data
Copy link

Choose a reason for hiding this comment

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

Is there an "official doc" where the contract with the watchdog is explained?
The goal is to know which field is mandatory, and which is not

Copy link
Member

Choose a reason for hiding this comment

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

Hi @pcorbel how is this question related to the Python template?

@@ -0,0 +1,48 @@
FROM armhf/python:3.6-alpine
Copy link

Choose a reason for hiding this comment

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

The stock image 'library/python:3.6-alpine' is compatible with ARM.
So we can use

FROM python:3.6-alpine

and if the image is build on ARM, the ARM version will be pulled.
Tested on Raspberry pi OK 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I had no idea about that! You've tested it on the Raspberry Pi already?

Copy link

Choose a reason for hiding this comment

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

Yes I've tested and it ran successfully

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried this and it failed. How were you able to run the image on your Raspberry Pi? Did you have to make any other changes?

Copy link
Member

Choose a reason for hiding this comment

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

Tried what / what failed? Give steps please so we can try too.

Copy link
Member

@alexellis alexellis Mar 3, 2019

Choose a reason for hiding this comment

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

This appears to work for me on armhf:

docker run -ti python:3.6-alpine /bin/sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried building the template with python:3.6-alpine and deploying it on the rpi, but the watchdog fails with the message Error reading stdout: EOF.

@@ -0,0 +1,68 @@
from flask import Flask, request, jsonify
Copy link

Choose a reason for hiding this comment

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

No header?
I think it would be a good practice to put

#!/usr/bin/env python
# -*- coding: utf-8 -*-

everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unfamiliar with this so I'll take a look into it

Copy link
Member

Choose a reason for hiding this comment

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

We haven't done this elsewhere so please propose it generically @pcorbel in a separate issue.

@@ -0,0 +1,48 @@
FROM python:3.7-alpine
Copy link

Choose a reason for hiding this comment

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

Why base python3-http on 3.7 and python-http-armhf on 3.6?
I think it would be a good idea to sync the two of them

@alexellis
Copy link
Member

Hi, is this still WIP?

Signed-off-by: Kevin Turcios <kevin_turcios@outlook.com>
@kturcios kturcios changed the title [WIP] Update the handler to match style of golang-http-template Update the handler to match style of golang-http-template Mar 3, 2019
@kturcios
Copy link
Contributor Author

kturcios commented Mar 3, 2019

It should be ready now

README.md Outdated

Python OpenFaaS template with Flask
The Python Flask templates make use of the incubator project [of-watchdog](https://github.com/openfaas-incubator/of-watchdog) to handle higher throughput than the [classic watchdog](https://github.com/openfaas/faas/tree/master/watchdog) due to the process being kept warm.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is technically correct but confusing / too-much-info for the people who may read it.

to handle higher throughput than the classic watchdog due to the process being kept warm.

README.md Outdated
faas template pull https://github.com/openfaas-incubator/python-flask-template
faas new --list
Languages available as templates:
Templates available under this repository:
Copy link
Member

Choose a reason for hiding this comment

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

under->in

README.md Outdated
- python27-flask
- python3-flask
```
- python3-flask-armhf
- python3-http*
Copy link
Member

Choose a reason for hiding this comment

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

python3-http* - why the suffix *?

README.md Outdated

Generate a function with one of the languages:
Notes:
- The templates listed with an asterik are the only ones that support additional control over the HTTP response and provide access to HTTP request details.
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary to explain this, just show it in the examples.

Copy link
Member

Choose a reason for hiding this comment

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

..


# Wait a couple of seconds then:
For example, returning a dict object type will automatically attach the header `Content-Type: application/json` and returning a string type will automatically attach the `Content-Type: text/html, charset=utf-8` for you.
Copy link
Member

Choose a reason for hiding this comment

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

Should we mention flask given that this is not visible to the user? the template will?

Copy link
Member

Choose a reason for hiding this comment

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

..

README.md Outdated

echo -n content | faas invoke myfunction
## Example usage
### Custom Status Codes and Response Bodies
Copy link
Member

Choose a reason for hiding this comment

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

Why is Bodies capitalisedbutExample usage` isn't?

Sentence case should be fine for both

Signed-off-by: Kevin Turcios <kevin_turcios@outlook.com>

ARG ADDITIONAL_PACKAGE
# Alternatively use ADD https:// (which will not be cached by Docker builder)
RUN apk --no-cache add curl ${ADDITIONAL_PACKAGE} \

Choose a reason for hiding this comment

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

As a user, I'd like to be able to install build dependencies and have them removed after pip installs dependencies.

Is that possible?

Copy link
Member

Choose a reason for hiding this comment

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

Some build dependencies generate .so libraries which are needed in the deployment / runtime container.

self.headers = request.headers
self.method = request.method
self.query = request.args
self.path = request.path
Copy link

@dschulten dschulten Apr 4, 2019

Choose a reason for hiding this comment

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

Please also ad request.files and request.form - they are needed for multipart/form-data requests. The request.files attribute is an ImmutableMultiDict of FileStorage (which is a file-ish ducktype), and form is an ImmutableMultiDict of strings which represent the textual multiparts which have no filename in their content-disposition. Maybe make a plain dictionary of lists from the MultiDict, to abstract away the Flask api.

That would make my pull request obsolete.

Copy link
Member

Choose a reason for hiding this comment

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

That won't be added at this time, but feel free to propose it as an issue.

Choose a reason for hiding this comment

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

👍

@alexellis alexellis merged commit 79e29c4 into openfaas:master Apr 4, 2019
@alexellis
Copy link
Member

@kturcios I'm going to merge this and would like the comments followed-up in a separate PR.

Please also upgrade the watchdogs to the latest possible version.

Thank you for working on this template 💯

@kturcios kturcios deleted the event-context-model branch April 6, 2019 17:48
@alexellis alexellis removed this from Review in Triage - Code/Review/Merge Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: should we revert to Flask dev server? Match style of golang-http / java template
5 participants