-
Notifications
You must be signed in to change notification settings - Fork 62
Updated Dockerfile in preparation for public Docker image #826
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
Updated Dockerfile in preparation for public Docker image #826
Conversation
9cc8491 to
ce05c0e
Compare
| RUN set -x && apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y \ | ||
| ca-certificates && \ | ||
| rm -rf /var/lib/apt/lists/* |
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 confirm that this container has the alpine equivalent of ca-certificates?
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.
ca-certificates exists in Alpine repo but is missing in the image. I will push a new revision that has this fixed.
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.
Thanks.
| FROM --platform=$BUILDARCH scratch AS dist | ||
| COPY ./dist/nix_linux_amd64_v1/temporal /dist/amd64/temporal | ||
| COPY ./dist/nix_linux_arm64/temporal /dist/arm64/temporal | ||
|
|
||
| WORKDIR /app | ||
| FROM alpine:3.22 | ||
| ARG TARGETARCH | ||
| COPY --from=dist /dist/$TARGETARCH/temporal /usr/local/bin/temporal |
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't we do this in one step? Why do we need a separate diststep?
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.
The dist step acts as a manual per-architecture mapping to the right binary. Docker doesn't support conditional COPY so due to the differences in Go and Docker target naming, it would be complicated to do in a single step.
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.
Not sure I follow, can't this step be COPY ./dist/$TARGETARCH/temporal /usr/local/bin/temporal?
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 won't work because the directory name is in the wrong format (arch is amd64_v1 in Goreleaser but amd64 in Docker).
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.
So use a build arg to say where to copy from instead?
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 wouldn't work for building multiplatform image with a single docker build command as all platforms would receive the same argument. I guess we could have a Docker bake file or a shell script wrapper or something but I think we should wait with that kind of stuff until we have proper release automation so that we don't do any throwaway work, and keep this PR as simple as possible. Especially since the extra step doesn't affect the final container image.
| COPY ./dist/nix_linux_arm64/temporal /dist/arm64/temporal | ||
|
|
||
| WORKDIR /app | ||
| FROM alpine:3.22 |
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.
Doesn't this also need a --platform flag?
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.
No. --platform defaults to $TARGETPLATFORM, which is what we want here. dist uses $BUILDPLATFORM so it can be potentially cached for multiplatform builds.
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 see.
| COPY ./dist/nix_linux_arm64/temporal /dist/arm64/temporal | ||
|
|
||
| WORKDIR /app | ||
| FROM alpine:3.22 |
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.
So we don't test against this OS very well FWIW, though Go is self-contained and doesn't use libc/musl so it should be fine. But just curious why alpine instead of e.g. debian:bookworm-slim or maybe even gcr.io/distroless/static-debian12 or others often seen in the wild? It seems the latter (https://github.com/GoogleContainerTools/distroless) is quite common for this use case.
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 was basing the image on our admin-tools image which uses Alpine. I'm not very well versed in Docker conventions, but it works, and Alpine is only 4MB compared to debian:bookworm-slim's 28MB. But I don't have a strong opinion here.
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.
👍 Curious about the distroless one since it's fairly common these days (e.g. it's what Docker uses in their guide sample at https://docs.docker.com/guides/golang/build-images/#multi-stage-builds). But Alpine is fine too. Not a blocker.
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 was thinking of recommending distroless too here. Alpine is a bit nicer if you ever want a shell to inspect the image, we may want that if we add support for config files later. Agree it's not a blocker.
| Docker image build requires [Goreleaser](https://goreleaser.com/) to build the binaries first, although it doesn't use | ||
| Goreleaser for the Docker image itself. |
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.
although it doesn't use Goreleaser for the Docker image itself.
So was looking at https://goreleaser.com/customization/docker/ and it seems they won't publish for you, is that correct? If so, it makes sense we don't want to rely on it.
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's not documented but it can in fact push to Dockerhub. But since it's undocumented I don't want to rely on it. The bigger reason is that it can't build a true multiplatform image, it needs to build them separately for each platform and then combine them with a manifest, and it can only do one tag at a time it seems (and we need two - the version and the latest). It too is very poorly documented, and also we're using Goreleaser v1 which they don't publish docs for anymore (I had to use Web Archive). Overall, it feels very not worth it.
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.
Wondering why, with this conversation seemingly open-ended, the MR was merged.
| COPY ./dist/nix_linux_arm64/temporal /dist/arm64/temporal | ||
|
|
||
| WORKDIR /app | ||
| FROM alpine:3.22 |
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.
👍 Curious about the distroless one since it's fairly common these days (e.g. it's what Docker uses in their guide sample at https://docs.docker.com/guides/golang/build-images/#multi-stage-builds). But Alpine is fine too. Not a blocker.
What was changed
Updated Dockerfile to be based on Alpine and to package binaries produced by Goreleaser. Added instructions to CONTRIBUTORS.md on how to build Docker image.
Note that this PR does NOT contain any automation for Docker image publishing. Release 1.4.0 will be published manually after this PR is merged.
Why?
Feature request: #316
Checklist
Internal release instructions will need to be updated.