-
Notifications
You must be signed in to change notification settings - Fork 267
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
Production ready docker #919
Conversation
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.
Some small remarks :)
Dockerfile
Outdated
@@ -1,5 +1,8 @@ | |||
FROM python:3.7-alpine | |||
FROM python:3.9-alpine |
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.
What about using 3.10-alpine? Is it because we don't officially support it yet?
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.
Exactly, didn't test with 3.10, but once tests are updated we should update this
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.
You can go ahead with 3.10 now that #921 was merged
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.
Alright !
Dockerfile
Outdated
echo "**** create runtime folder ****" && \ | ||
mkdir -p /etc/ihatemoney &&\ | ||
echo "**** install pip packages ****" && \ | ||
pip install --no-cache-dir gunicorn pymysql && \ |
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.
How about installing psycopg2
too, so you can use a postgres database with the docker image?
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 tried adding it afterwards by running from a simple local Dockerfile like this:
FROM ihatemoney/ihatemoney:5.1.1
RUN pip install --no-cache-dir psycopg2
But building fails, not sure why - I don't really have experience using python.
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.
Can you tell us how it fails, maybe you have some logs?
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.
Sure, I've opened a proper Issue: #924
Well done for the lightweight postgresql integration :) I have one nitpick: tests are run with constrained versions of psycopg2 and pymysql defined in setup.py, currently:
It would be good to use the same constraints in the Docker image to avoid surprises. I suggest moving these two dependencies out of the |
Good call, done ! |
Looks good, thanks! |
I updated the dockerhub readme to include tags description, compose example and env list : https://hub.docker.com/r/ihatemoney/ihatemoney |
Also, we may want to trigger another deploy to make PORT,PGID and PUID options available to |
You mean overwriting the last tagged release on the dockerhub? That does not look like a good idea. I guess we simply need to do a release soon. |
That is what I meant ;) Creating a new release after 5.1.1, thus pushing a new |
* /healthcheck endpoint usefull for monitoring, ci test also uses this * customizable PORT with environment variable * customizable PUID/PGID, reduce attack surface and allow better integration in rootless environments * size optimization * update to python 3.10 * add postgresql compatibility * PUID/PGID default as root to not break current user environments
Some changes in this PR to make the docker image production ready :
PUID/PGID default as root to not break current user environments