Conversation
alanshaw
left a comment
alanshaw
left a comment
There was a problem hiding this comment.
LGTM - left some suggestions.
|
|
||
| ### Pull Requests | ||
|
|
||
| PR builds should validate that the Dockerfile compiles but need not push images anywhere. Building on a single platform (amd64) is sufficient here — the goal is fast feedback, not architectural completeness. |
There was a problem hiding this comment.
Is there any chance that building for a different arch could fail? My guess is no, or at least very unlikely, but if we're concerned then we should probably build on both.
|
|
||
| ## CI/CD Considerations | ||
|
|
||
| The workflow should live at `.github/workflows/publish-ghcr.yml` and be named `Container`. |
volmedo
left a comment
volmedo
left a comment
There was a problem hiding this comment.
Looks good overall. I left some suggestions.
| The production target should be small, fast, and boring: | ||
|
|
||
| - Stripped binary (`-ldflags="-s -w"`) | ||
| - Minimal base image (`debian:bookworm-slim`) |
There was a problem hiding this comment.
For Go I'd go with scratch for a really minimal image with the smallest attack surface possible. The caveat is that scratch has nothing in it, so it requires copying CA certificates from the builder image (we currently do this in our services deployed via storoku), and setting a non-root image up is a bit more involved.
Alternatively, gcr.io/distroless/static could be a good middle ground, as it is small but comes with CA certificates and a non-root user pre-baked in the image.
Of course, both alternatives work only for statically-linked binaries. But that is what we are doing currently.
There was a problem hiding this comment.
Incorporated with a twist. The RFC now standardizes on alpine:latest as the single base image — it's ~5MB, includes wget and CA certificates, and provides a shell for Docker Compose healthchecks as well as entry point scripts which Piri and IPNI require.
|
|
||
| - Stripped binary (`-ldflags="-s -w"`) | ||
| - Minimal base image (`debian:bookworm-slim`) | ||
| - Only what's needed to run and health-check: `ca-certificates`, `curl` |
There was a problem hiding this comment.
To run a healthcheck on a different service? Having curl installed in a prod image it's an uncommon requirement IMO.
There was a problem hiding this comment.
Standardizing on wget instead which we get for free via #83 (comment)
|
|
||
| The precise selection of debugging tools is a matter of taste. The principle is not: include everything that may be required. | ||
|
|
||
| ### Non-root user |
|
|
||
| The key insight: `$BUILDPLATFORM` is the CI runner's architecture (x86_64), and `$TARGETPLATFORM` is the final image's architecture. Go handles cross-compilation natively, so the compiler runs fast while producing binaries for whatever architecture you need. | ||
|
|
||
| QEMU remains necessary for the runtime stages, where `apt-get` and friends must execute on the target architecture. These operations are lightweight compared to compilation, so the tradeoff is favorable. |
There was a problem hiding this comment.
Avoiding the RUN command would also avoid the need of emulation completely. CA certificates can be installed in the builder and copied over to the runtime image (or use an image that comes with them pre-installed, such as distroless/static). Getting curl would be trickier though, but I'm not sure that should be a requirement.
There was a problem hiding this comment.
switching to alpine:latest should address this. Sorry was over complicating things here.
| | `<sha>` | prod | No | Specific commit (production build) | | ||
| | `<sha>-dev` | dev | No | Specific commit (dev build) | |
There was a problem hiding this comment.
These are short SHAs, correct? Might be worth making it explicit.
There was a problem hiding this comment.
yea, now explicitly stated
Add docker container release process Follows outline proposed in storacha/RFC#83 Co-authored-by: Petra Jaros <peeja@peeja.com>
3 participants
Preview