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 starlette documentation #1461

Merged
merged 5 commits into from
Jan 6, 2024

Conversation

webb-ben
Copy link
Member

@webb-ben webb-ben commented Dec 27, 2023

Overview

This PR adds the necessary components to deploy the pygeoapi Docker image with the startlette framework. This can be done by updating two necessary environment variables. Not sure WSGI_APP_FRAMEWORK is the best env variable name seeing as Starlette is ASGI

Related Issue / Discussion

#1460, #1400, #1111, #227

Additional Information

Contributions and Licensing

(as per https://github.com/geopython/pygeoapi/blob/master/CONTRIBUTING.md#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to pygeoapi. I confirm that my contributions to pygeoapi will be compatible with the pygeoapi license guidelines at the time of contribution.
  • I have already previously agreed to the pygeoapi Contributions and Licensing Guidelines

@webb-ben webb-ben requested a review from tomkralidis December 27, 2023 18:58
@webb-ben webb-ben changed the title Add Starlette capacity to Docker image Add Starlette to Docker image Dec 28, 2023
@webb-ben webb-ben requested a review from francbartoli January 3, 2024 19:46
Copy link
Contributor

@francbartoli francbartoli left a comment

Choose a reason for hiding this comment

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

Thanks @webb-ben, to be honest with you, if the intent is to provide a ready to go image for production purposes then I really don't understand why we are collapsing two different frameworks into the same image.
Also, as you mentioned starlette is ASGI and so I still really don’t understand why we should scale the workers number within the container rather than doing that at infrastructure level.
I can only understand this addition only if the goal here is to provide a uniform way to make the developer experience easier but I understood in the #1252 this is not the only intent.
Certainly it wouldn’t be the recommended way to deploy other ASGI framework like FastAPI (which is based on Starlette) on containers, k8s or even in serverless functions. In these case I should need a proper Dockerfile to build the optimised image from scratch.
Sorry, I might be wrong, but my feeling is more or less the same as I’ve got with #1400.
I'd like to help here but I don't have any idea other than having different Dockerfiles based on purposes/frameworks. Maybe we could start thinking about a separate pygeoapi-devops repository under the Geopython organisation.

@webb-ben
Copy link
Member Author

webb-ben commented Jan 4, 2024

@francbartoli I do not have much experience deploying ASGI. This PR comes from my testing of the Admin API and following the pygeoapi docs on deploying pygeoapi with gunicorn and starlette which are out of date.

More than happy to reduce this PR to just fix the documentation if that is the intended purpose. This PR exists in lieu of proper Django and Starlette images for pygeoapi.

@francbartoli
Copy link
Contributor

@webb-ben at the same time I don't want to block it, thanks

@tomkralidis
Copy link
Member

+1 to reduce PR to documentation. Thanks.

@webb-ben webb-ben changed the title Add Starlette to Docker image Update starlette do umentation Jan 5, 2024
@webb-ben webb-ben changed the title Update starlette do umentation Update starlette documentation Jan 5, 2024
@webb-ben
Copy link
Member Author

webb-ben commented Jan 5, 2024

@tomkralidis ready to merge. Although I am not sure we publish to ghcr anymore.

@tomkralidis tomkralidis merged commit 0a47ad2 into geopython:master Jan 6, 2024
3 checks passed
@webb-ben webb-ben deleted the starlette-docker branch July 17, 2024 19:58
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.

3 participants