-
Notifications
You must be signed in to change notification settings - Fork 422
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 Dockerfile
& docker-compose
for faster, usable builds
#2942
Conversation
Takes advantage of layer caching for kinto admin build and python dependency downloading
@@ -7,7 +7,7 @@ jsonpatch==1.32 | |||
jsonschema==4.4.0 | |||
logging-color-formatter==1.0.2 | |||
newrelic==7.2.4.171 | |||
psycopg2-binary==2.9.3 | |||
psycopg2==2.9.3 |
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.
This change is because kinto[postgresql]
requires psycopg2
(not psycopg2-binary
)
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.
psycopg2-binary is psycopg2 precompiled, we should keep the binary version IMHO.
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.
psycopg2-binary is psycopg2 precompiled, we should keep the binary version IMHO.
I would prefer that as well, but I think in order to do this, we'll have to change psycopg2
to psycopg2-binary
in setup.cfg
. Would that be acceptable? I didn't know if there was a good reason we needed to build psycopg2
when installing kinto
.
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 makes sense to align the dependencies names. Shall we try psycopg2-binary
everywhere?
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.
Shall we try
psycopg2-binary
everywhere?
Yep, we certainly can. I was just ensuring there wasn't a good reason to build psycopg2
. From their docs:
If you are the maintainer of a published package depending on psycopg2 you shouldn’t use psycopg2-binary as a module dependency. For production use you are advised to use the source distribution.
and
Warning
The psycopg2 wheel package comes packaged, among the others, with its own libssl binary. This may create conflicts with other extension modules binding with libssl as well, for instance with the Python ssl module: in some cases, under concurrency, the interaction between the two libraries may result in a segfault. In case of doubts you are advised to use a package built from source.
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.
@@ -7,7 +7,7 @@ jsonpatch==1.32 | |||
jsonschema==4.4.0 | |||
logging-color-formatter==1.0.2 | |||
newrelic==7.2.4.171 | |||
psycopg2-binary==2.9.3 | |||
psycopg2==2.9.3 |
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 makes sense to align the dependencies names. Shall we try psycopg2-binary
everywhere?
This PR:
This is so that we can cache the layers responsible for building
kinto-admin
and installing the dependencieskinto
relies on to build itself. This also leads to an overall smaller image by the end of the build.Before:
After
It also:
docker-compose
file to be usable again, since services must be nested under a top-levelservices
key