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

Clarify use and purpose of templates in this repo #34

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

alexellis
Copy link
Member

Clarifies expectations for usage of the templates in this
repo along with an update to the python3-flask template so that
it provides a better upgrade path.

Examples added for python3-flask to show how to return a HTTP
error or status code.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) alexellis2@gmail.com

@alexellis alexellis force-pushed the openfaasltd/clarify-templates branch from cb2657b to 89b0049 Compare June 9, 2020 13:54
@@ -22,7 +23,13 @@ def fix_transfer_encoding():
@app.route("/", defaults={"path": ""}, methods=["POST", "GET"])
@app.route("/<path:path>", methods=["POST", "GET"])
def main_route(path):
ret = handler.handle(request.get_data())
raw_body = os.getenv("RAW_BODY")
Copy link
Member Author

Choose a reason for hiding this comment

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

ret = handler.handle(request.get_data())
raw_body = os.getenv("RAW_BODY")

as_text = True
Copy link
Member Author

Choose a reason for hiding this comment

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

To enable easier migration from the python3 template

@alexellis alexellis force-pushed the openfaasltd/clarify-templates branch from 89b0049 to 853c27e Compare June 9, 2020 14:26
raw_body = os.getenv("RAW_BODY")

as_text = True
if raw_body == "True":
Copy link
Member

Choose a reason for hiding this comment

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

perhaps this would be useful here https://docs.python.org/3/distutils/apiref.html#distutils.util.strtobool

Convert a string representation of truth to true (1) or false (0).

True values are y, yes, t, true, on and 1; false values are n, no, f, false, off and 0. Raises ValueError if val is anything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes sounds reasonable to me

Copy link

@Jeff-Lowrey Jeff-Lowrey left a comment

Choose a reason for hiding this comment

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

There are several places in the README where a 201 is returned instead of a 200.

Is there a specific reason for this?

For example

Successful response status code and string response body

python
def handle(event, context):
return {
"statusCode": 201,
"body": "Object successfully created"
}

@alexellis
Copy link
Member Author

Yes - to show that you can alter the HTTP status code @Jeff-Lowrey

@Jeff-Lowrey
Copy link

Jeff-Lowrey commented Jun 9, 2020

@alexellis Ok, here's the bit that itches at me.

The two examples for custom status codes and response bodies in the example usage for the -http templates seem a little... I dunno - off balance?

The first one shows a standard response code and (I guess json counts as?) a custom response body.
The second one shows a custom response code and (I guess plain text counts as?) a standard response body.

I'd be happier, I think, if the first one said

Successful response status code and custom JSON response body

and the second said

Custom response status code and standard string response body

Or otherwise something to emphasize the point of the two different examples. One is a custom body and one is a custom response code.

I'm sure I'm way over thinking this.

current:

Successful response status code and JSON response body

def handle(event, context):
    return {
        "statusCode": 200,
        "body": {
            "key": "value"
        }
    }

versus

Successful response status code and string response body

def handle(event, context):
    return {
        "statusCode": 201,
        "body": "Object successfully created"
    }

Clarifies expectations for usage of the templates in this
repo along with an update to the python3-flask template so that
it provides a better upgrade path.

Examples added for python3-flask to show how to return a HTTP
error or status code.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
@alexellis alexellis force-pushed the openfaasltd/clarify-templates branch from 853c27e to a15e2c6 Compare June 10, 2020 08:43
@alexellis
Copy link
Member Author

@Jeff-Lowrey I'm sorry I don't quite get what point you're trying to make? Is it that you would like to see another HTTP code other than 201 used in examples? What would that add?

@alexellis alexellis merged commit 7411356 into master Jun 10, 2020
@alexellis alexellis deleted the openfaasltd/clarify-templates branch June 10, 2020 08:44

Those templates named `python*-http*` are designed to offer full control over the HTTP request and response. Flask is used as an underlying framework.

The `witness` HTTP server is used along with Flask for all templates.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is a typo, it is waitress not witness

@Jeff-Lowrey
Copy link

@Jeff-Lowrey I'm sorry I don't quite get what point you're trying to make? Is it that you would like to see another HTTP code other than 201 used in examples? What would that add?

@alexellis I'm trying to clarify the descriptive text of each example in the -http section to show why one has a 201 and one has a 200. I know that the intent is to show custom return codes and custom message bodies.

It's just, at least to me, not glaringly obvious from the descriptive text what each example is showing - it's only really clear when I read the code. Without explicitly noticing the result code, the descriptions say that example one has a JSON reply body and example two has a string reply body.

The text in the JSON reply body does say something that matches the 201. But, again, this is not glaringly obvious from the description of the example.

Maybe I'm over thinking this. But it did confuse me at first reading even though I know about setting the statusCode field. So it might be more confusing to someone that doesn't know about using the status code.

I don't think this is a "must fix" issue. Just something that popped out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants