-
Notifications
You must be signed in to change notification settings - Fork 3
Add doc on deployment #52
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Anuraag Agrawal <[email protected]>
stefanvanburen
left a comment
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, just a suggestion on the CORS section
docs/deployment.md
Outdated
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=["*"], | ||
| allow_methods=["GET", "POST"], | ||
| allow_headers=["*"], | ||
| ) |
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.
the * seems a little loose to me; could we:
- for
allow_origins, add a comment to "replace with your domain" (similar to cors-go example) - add comments to
GETandPOSTfor why they're needed (GET for connect, POST for connect/gRPC) - pull in the headers w/ comments from cors-go?
We may eventually want to have a cors-go equivalent in python for ease of adding these headers, but adding these seems like enough for now.
| using HTTP/1.1 without issue. Two servers have long been in use with HTTP/1.1 and have a proven track | ||
| record. If you are unsure what server to use, it is generally a safe bet to use one of them: | ||
|
|
||
| - [gunicorn](http://www.gunicorn.org/) for WSGI applications |
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.
maybe https:// is better?
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.
I guess they'll be in trouble when Chrome requires it - they don't seem to support https...
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.
When I commented, I thought it connected via https, but it seems I was mistaken. sorry...
| - [uvicorn](https://uvicorn.dev/) for ASGI applications | ||
|
|
||
| ### HTTP/2 | ||
|
|
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.
I'm not sure and forgot about daphne. Should we mention daphne as well?
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.
Unfortunately Daphne's http2 support doesn't work with bidi so I think it's confusing to add to this list. And without it, I think uvicorn is a better choice so left it out.
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.
Unfortunately Daphne's http2 support doesn't work with bidi so I think it's confusing to add to this list. And without it, I think uvicorn is a better choice so left it out.
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, I see.
|
|
||
| - [granian](https://github.com/emmett-framework/granian) | ||
| - [hypercorn](https://hypercorn.readthedocs.io/en/latest/) | ||
| - [pyvoy](https://github.com/curioswitch/pyvoy) |
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.
I'd love to improve https://github.com/i2y/tsuno so it can be on this list someday! (just a comment)
docs/deployment.md
Outdated
| ## Application Servers | ||
|
|
||
| Connect services are standard ASGI or WSGI applications that can be run with an off-the-shelf Python | ||
| application server. This means generally, any experience in running a Python application can be applied |
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.
| application server. This means generally, any experience in running a Python application can be applied | |
| application server. Generally, any experience in running a Python application can be applied |
docs/deployment.md
Outdated
| # Mount your Connect application | ||
| ``` |
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.
I think we should explain how to mount onto Starlette, here.
| Connect services are standard ASGI and WSGI applications so any CORS middleware can be used to | ||
| enable it. |
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.
We should probably provide a CORS interceptor, so that we don't give the false impression that users need to use Starlette for CORS support.
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.
I think it's ok to add an interceptor but also feel it's an HTTP concern rather than RPC and could be best left to other libraries. I changed to using asgi-cors to stress this is about using off-the-shelf middleware, not starlette's routing. What do you think?
Signed-off-by: Anuraag Agrawal <[email protected]>
After much testing of various servers, I finally found the words for a deployment doc. Happy for any suggestions as this was the least mechanical of any of the docs