-
Notifications
You must be signed in to change notification settings - Fork 12
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 production dockerfiles and github action #794
Conversation
docker/docker-compose.prod.yml
Outdated
phpreport-app: | ||
build: | ||
context: ../ | ||
dockerfile: docker/dev.app.Dockerfile | ||
container_name: phpreport-app | ||
ports: | ||
- "8000:8000" | ||
depends_on: | ||
- db |
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 better to remove this container, we are not planning to deploy it to prod like this and if we need to do it then we'd also need to make a production dockerfile for it. The dev one spawns the built-in php web server and it's not production ready.
a1500e9
to
01cfa17
Compare
docker/docker-compose.prod.yml
Outdated
version: "3" | ||
services: | ||
db: | ||
image: postgres:13.9 |
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.
It's better practice to just stick to a major version, ie postgres:13
, as all binaries throughout the 13.x branch are guaranteed to be data/abi compatible, and the specific 13.9 minor version is not maintained nor receives any updates, with 13.14 being the current one right now.
(Also, while 13 will be supported until Sept 2025, it's most probably possible to bump to 16 without any schema changes or so).
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.
docker/docker-compose.prod.yml
Outdated
env_file: | ||
- ../frontend/.env.local | ||
ports: | ||
- "5173:3000" |
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.
Wondering why 3000 and not 5173?
image: postgres:13.9 | ||
container_name: phpreport-db | ||
ports: | ||
- "5432:5432" |
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.
Does this port need to be exposed at all? Is it intended to be used by anything besides api and frontend?
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, thanks
- Make various changes to solve issues at build time
01cfa17
to
f8322bb
Compare
This PR adds production-ready dockerfiles to the project, as well as a github action to build images on pushes to main.
The production dockerfiles are based on the following:
Next: https://github.com/vercel/next.js/blob/canary/examples/with-docker/Dockerfile
FastApi: https://fastapi.tiangolo.com/deployment/docker/#dockerfile
Several changes were needed on the frontend due to errors that occurred at build time. Some were small, such as removing references to unused components, but others were larger, such as moving the authOptions out into its own file and moving the action for the template modal to a form element I added around the Sheet (instead of on the Sheet itself).
Additional note for context that is outside of the PR: There was some configuration required in Github for the containers/packages, including some permissions. I also had to manually build and push the images to Github first so that I could manage the permissions that Actions needed.