Better Dockerfile #645
Better Dockerfile #645
Conversation
| RUN \ | ||
| curl --silent https://www.postgresql.org/media/keys/ACCC4CF8.asc | apt-key add && \ | ||
| echo "deb http://apt.postgresql.org/pub/repos/apt/ `lsb_release -cs`-pgdg main" | tee /etc/apt/sources.list.d/pgdg.list && \ | ||
| apt-get update && apt-get install -y postgresql-client |
There was a problem hiding this comment.
Pgweb absolutely needs the postgres client package since it utulizes pg_dump under the hood for exporting data dumps.
There was a problem hiding this comment.
Oh, ok, I didn't know that, thanks.
Dockerfile
Outdated
| apt-get autoremove --yes && \ | ||
| rm -rf /var/lib/{apt,dpkg,cache,log}/ | ||
| FROM scratch AS wait | ||
| ADD https://raw.githubusercontent.com/eficode/wait-for/v2.2.4/wait-for /usr/bin/wait-for |
There was a problem hiding this comment.
I would advise against pulling a dependency from a random Github endpoint, especially if such dependency is a fork. Previously I've used a similar tool, though it seems like we could just grab the contents of https://github.com/eficode/wait-for/blob/master/wait-for (MIT licensed) and use in the repo moving forward.
There was a problem hiding this comment.
I don't understand what you mean with "random [...] endpoint". Would you rather use a different tool?
Dockerfile
Outdated
| # ------------------------------------------------------------------------------ | ||
| # Release Stage | ||
| # ------------------------------------------------------------------------------ | ||
| FROM busybox:musl |
There was a problem hiding this comment.
This needs to support current build targets as defined in here. https://github.com/sosedoff/pgweb/blob/master/.github/workflows/docker.yml#L35
As for having a very slim base image - there might be some other issues (like the missing postgres client package) and unfortunately I did not write down any notes since the last time I had to deal with this. A proper E2E test using Docker image is probably in order, however definitely out of scope of the PR.
|
|
||
| RUN \ | ||
| apt-get update && \ | ||
| apt-get install -y ca-certificates openssl netcat curl gnupg lsb-release && \ |
There was a problem hiding this comment.
Missing ca-certs and openssl packages might have an impact on pgweb connectivity capabilities, I did run into similar issues before, however i don't recall the details.
Also, netcat and curl as super useful when you need to drop into container and troubleshoot things quickly.
There was a problem hiding this comment.
Should pgweb connect to external servers while running?
e9878a9 to
89e7745
Compare
10849dd to
362686b
Compare
|
I reduced the effects of this PR by dropping some commits and re-adding some missing packages. |
|
Thanks, i'll review |
Fix #644.
I created a new stage to handle the key, so that many tools do not end up in the final image, then I further simplified both the build and release stage. This reduced the image size from ~200 MB to ~160 MB, and reduced the build time as well. I also removed
apt-key, which is deprecated, used quiet mode for apt, and avoided installed recommended packages.I dropped the postgres client as well, as it is needed just when running the tests. By doing that, the only external dependency is
nc, which is provided by a minimalbusybox:muslimage (<1 MB). The final image ~15 MB, which is ~92 % less of the original image. Since I was not sure if this is the desired result, I kept the commit split.Since the binary is now built with
CGO_ENABLED=0, there is no need to use a golang image based on Debian, so the alpine version is used instead.Replacing
ncwith a different way to do a self-check would further reduce the image, but usingscratchas the base image could be problematic for some users, that are used to have a minimal shell within the container.As the last step, I added https://github.com/eficode/wait-for, a small POSIX shell script, to wait for Postgres without having to rely on specific docker compose functionalities, which are not available in other systems.
depends_ondoes not even work in some Docker environments. I replaced the health check that usedncwithwait-for, which tries to establish a http connection by default.The software works as usual, but a review would be helpful.
I also added
build: .to allow local builds usingdocker compose.The current version requires 27 seconds to be built, and its size 197 MB (
docker system prune -af && time rootlesskit docker build --no-cache .). This includes the image pull from the server. In my case, I am using a gigabit connection, so the impact of using larger images is small, but it could affect other users more.Using the same criteria, the version proposed in this PR takes 20 seconds to be built, and its size is 16 MB.
Possible improvements for the future:
.gitshould not be copied when building the image, andgitinstallation could be skipped as well;RUN --mount=type=cache,mode=0755,target=/go/pkg/modand similar to avoid downloading dependencies multiple times when there is a minor change;