-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add docker buildx make target #3213
Conversation
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #3213 +/- ##
==========================================
- Coverage 95.95% 95.92% -0.04%
==========================================
Files 242 242
Lines 14786 14786
==========================================
- Hits 14188 14183 -5
- Misses 520 524 +4
- Partials 78 79 +1
Continue to review full report at Codecov.
|
@@ -21,3 +21,13 @@ create-debugimg: | |||
--build-arg golang_image=$(GOLANG_IMAGE) \ | |||
--platform=$(PLATFORMS) \ | |||
docker/debug | |||
|
|||
.PHONY: prepare-docker-buildx | |||
prepare-docker-buildx: |
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 is a nice idea. It would be even nicer if it was a seamless experience for local dev. For example, if I run the following locally:
make docker-images-jaeger-backend
I still get the error, because the local registry isn't started as part of this make target:
error: failed to solve: rpc error: code = Unknown desc = failed to do request: Head "http://localhost:5000/v2/baseimg_alpine/blobs/sha256:48167268581ca7f86ce8f794a24d144b0509556cf4c584eec9e95afebe88bcc6": dial tcp [::1]:5000: connect: connection refused
Given the only time we need the registry is when we do a push
, would it make sense to add this target as a dependency to both create-baseimg
and create-debugimg
? i.e.
create-baseimg: prepare-docker-buildx
...
create-debugimg: prepare-docker-buildx
...
That way, developers won't need to know to run make prepare-docker-buildx
for their docker builds to work.
If the above is feasible, it also means we could do away with the - run: make prepare-docker-buildx
within our github actions steps
, given create-baseimg
and/or create-debugimg
should be run.
The drawback I see is that make prepare-docker-buildx
isn't idempotent and I don't have any good suggestions right now to solve for it, except for making sure that make clean-docker-buildx
is always run at the end.
The other alternative is we document this extra manual step of make prepare-docker-buildx
clearly somewhere.
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.
btw. are you on macos? Were you able to run 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.
Yes, I'm on macos and successfully ran this make target together with other docker buildx.
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 didn't mind requiring devs to execute make prepare-docker-buildx
explicitly. It's quite easy to understand what needs to be run from the scripts.
I have made it idempotent in the last commit and removed prepare step from the ci jobs.
For instance IIRC install-tools
target is not invoked automatically before a target that uses tools.
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 quite easy to understand what needs to be run from the scripts.
yes, I agree, but knowing to where to find the scripts (I presume you mean the scripts run by github actions) seems less intuitive than looking in the Makefile since that's where the make targets are defined. For example, they may just want to build custom docker images for testing purposes and run make docker-images-jaeger-backend
, and not have any desire to replicate CI locally.
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.
When I am trying to build an unknown project I usually first go to CI config to see what is required to run. If there is a clear "prepare" step I can follow it and understand what needs to be done.
If the "prepare" for OOTB then it's good as well until it does not interfere with tools and services running on the host. In this case it might be the docker registry for instance.
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@albertteoh could you please review? |
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.
works like a charm, and nice to see less GHA config as well.
btw, looks like you need to rebase with master, meaning it would invalidate my approval. I'll re-approve after you rebase, if I'm still awake :)
@@ -21,3 +21,13 @@ create-debugimg: | |||
--build-arg golang_image=$(GOLANG_IMAGE) \ | |||
--platform=$(PLATFORMS) \ | |||
docker/debug | |||
|
|||
.PHONY: prepare-docker-buildx | |||
prepare-docker-buildx: |
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 quite easy to understand what needs to be run from the scripts.
yes, I agree, but knowing to where to find the scripts (I presume you mean the scripts run by github actions) seems less intuitive than looking in the Makefile since that's where the make targets are defined. For example, they may just want to build custom docker images for testing purposes and run make docker-images-jaeger-backend
, and not have any desire to replicate CI locally.
Updates #3209
This PR adds make target to configure docker buildx command. The idea is to allow contributors to run the build locally the same way how it runs on CI.