-
Notifications
You must be signed in to change notification settings - Fork 291
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
Multi-stage Dockerfile #1789
Multi-stage Dockerfile #1789
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.
This looks good, thank you. It also improves the situation regarding #1655. Using --chown=nginx:nginx
to workaround docker/for-linux#388 is interesting 👍
Currently the CI configuration only builds Docker images for master
and git tags, hence the Docker build did not trigger for this PR. To properly test this we should add a workflow that builds Docker images from Pull Requests (without pushing to dockerhub, credentials are not available outside protected branches/tags anyway).
Did you measure build times after/before the patch?
COPY --from=composer /app/shaarli shaarli | ||
RUN cd shaarli \ | ||
&& yarn install \ | ||
FROM node:12-alpine as app |
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.
as app
is confusing, shouldn't it be as node
or as 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.
It copy the .
to /tmp, I hope to distinguish it with package build only. The frontend
looks good. I used to use it as the final step, so I have renamed to app
, but this case no longer exists.
I don't know how to do this at this point, so welcome to help or push.
On an old machine, it exceeds 300s before, now about 100 seconds at node_modules, just < 10s if the node_modules keep, > 600s if |
I will try to get it working as soon as possible #1800 |
The changes in this PR are outdated/need to be rebased (or redone from scratch, which is probably easier). The main goal is to workaround docker/for-linux#388 (Docker bug from 2018 - The main change implemented here is the use of It should be investigated again to see if it improves build times. |
I expect it to improve slow
docker build
locally.alpine:3.14
updated. FYI:https://wiki.alpinelinux.org/wiki/Release_Notes_for_Alpine_3.13.0
https://wiki.alpinelinux.org/wiki/Release_Notes_for_Alpine_3.14.0
I expect the
COPY --chown=nginx:nginx shaarli_version.php /var/www/shaarli/
ensure a clean cache for release. Although this may not be effective fordev
branches, and an inconsistent results for the developer local. I still want to pass a variable orif
, only erase in cloud builds.If the
&& rm -rf ./node_modules
be removed, it will build quickly, but the docker image size + 50MiB, which seems not easy to solve.