-
Notifications
You must be signed in to change notification settings - Fork 981
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
Refactoring multiple parts of the dockerfile #10004
Conversation
bde93ee
to
45a97a4
Compare
Should it be added into GitHub Actions pipeline? I am interested to see how the changes improve the build time. |
I'm not sure this would speed up CI a lot, because I doubt GitHub CI keeps For the rest, the layers here should roughly take as long as before, it's refactored for readability and ease of use. In the end the operations we have are:
And even with a layer cache, it reduces bandwidth usage but from an human point of view, downloading 100ko with fiber or reading it from disk takes as long (0s). |
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 is really incredible work. Thank you @ewjoachim!
Our deployment pipeline doesn't currently have support for setting a target when building, but since the app ends up being default, that's not a problem!
# probably pin these too, and update them as we do anything else. | ||
RUN pip --no-cache-dir --disable-pip-version-check install --upgrade pip setuptools wheel | ||
# Pip configuration (https://github.com/pypa/warehouse/pull/4584) | ||
ENV PIP_NO_BINARY=hiredis PIP_DISABLE_PIP_VERSION_CHECK=1 |
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 completely escapes me why we do not use the binary releases for hiredis... but based on the date it seems to be related to redis/hiredis-py#69 (nice) which was seemingly only related to that 0.2.0 release.
Perhaps we should go back to using the wheels provided?
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.
Will address in follow up PR
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 reverts commit 6581b02.
* Remove the Dockerfile.static, replace with a --target * Use APT cache * Headers * Switch to set -eux * Add things to dockerignore * Add lockfile for pip & the likes * Add pip req alias files * Improve APT cache * Reorgnize apt deps * Make dev image, use pip req aliases, and envvars * Add base image to limit repetition * docker-compose use dev images * docker-compose use simple python images when possible * Dev is not pinned, must be installed separately * Remove unused DEVEL references * Build static image first, though it's not used by the dev image * Reorganize dev Co-authored-by: Ee Durbin <[email protected]>
…ypi#10020) This reverts commit 6581b02.
set -eux; \
style instead ofset -u \ &&
stylenotdatadog
&files
, should be smaller on disk/memoryThis is a good first step toward #9988
@ewdurbin I believe there might be implications on the deployment pipeline, though it might be nothing more than adding
--target=app
to the docker build. And even without that, I believe it would be the default.