chore: refactor to single multistage dockerfile with bake file #754
Conversation
docker-bake.hcl
Outdated
| } | ||
|
|
||
| target "node-release" { | ||
| inherits = ["node", "_release"] |
There was a problem hiding this comment.
the image built by release should be in format opr-node and opr-nodeplugin since that's what we do for public release. so you might want to override the tags in here.
docker-bake.hcl
Outdated
| } | ||
|
|
||
| group "disperser-group" { | ||
| targets = ["batcher", "disperser", "encoder", "retriever", "churner"] |
There was a problem hiding this comment.
Technically retriever and churner are not part of disperser
There was a problem hiding this comment.
ah good point will leave them as separate individual targets then.
|
|
||
| # RELEASE TARGETS | ||
|
|
||
| target "_release" { |
There was a problem hiding this comment.
curious why is it _release and not release ? something private or public?
There was a problem hiding this comment.
seems like it's not private, since you can still print it with docker buildx bake _release --print
Just indicative that it's not meant to be a real target.
| if: github.ref == 'refs/heads/master' | ||
| run: docker compose -f docker-compose-internal.yaml push |
There was a problem hiding this comment.
What do you think of making it a separate workflow and then deprecating this one?
There was a problem hiding this comment.
Feel it's not needed. If ever we have a problem with this we can git revert the PR.
| # syntax=docker/dockerfile:1 | ||
|
|
||
| # Declare build arguments | ||
| # TODO: this is only used for node image right now, should we also use it for nodeplugin? |
There was a problem hiding this comment.
Yeah we should use it for all
There was a problem hiding this comment.
just checked, nodeplugin doesn't even define those variables. Let's do it in a separate PR if it's ever needed. Probably add it to all binaries in one go.
| @@ -1,26 +0,0 @@ | |||
| FROM golang:1.21.1-alpine3.18 as builder | |||
There was a problem hiding this comment.
Similar to previous comment, do we want to make the PR additive first and then deprecate this stuff later?
There was a problem hiding this comment.
How would you test this if we don't remove it? I feel let's just test it, and if for some reason the releases don't work or there's something super buggy with it, then we just git revert.
There was a problem hiding this comment.
We can still test it because there would be two separate pipelines. I'm fine with reverting if there's problems.
samlaf
deleted the
feat-docker-refactor-single-multistage-file-with-bake
branch
Why are these changes needed?
All of the Dockerfiles were doing very similar things. Refactored to make use of buildkit multistage build (see this article for explanation). Thought there would be greater speedup but it went from ~2min20sec to ~1min30sec. Greatest advantage is cleanliness and maintainability. For eg with a single Dockerfile we can see the commonalities between the different targets and refactor as needed.
Also added a bake file with different targets. This replaces the docker compose files. Bake files are a new feature of docker which allows for grouping targets together. They also support extra useful features like inheritance, so easy to make release targets that add extra platforms for eg.
This refactor also makes it easier to inject into all images the same ldflags that we currently only inject into the node release build, but not into the nodeplugin or any other image. Easier to see patterns like this.
Testing
Have built the images manually successfully, but haven't done any testing with those images. Also haven't tested the pipeline changes that push to ghcr.