Skip to content
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

Merged
merged 4 commits into from
Aug 18, 2021

Conversation

pavolloffay
Copy link
Member

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.

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@pavolloffay pavolloffay requested a review from a team as a code owner August 17, 2021 12:33
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #3213 (9e2cfe8) into master (8cd84e7) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
pkg/config/tlscfg/cert_watcher.go 92.63% <0.00%> (-2.11%) ⬇️
cmd/query/app/server.go 94.11% <0.00%> (-1.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cd84e7...9e2cfe8. Read the comment docs.

@@ -21,3 +21,13 @@ create-debugimg:
--build-arg golang_image=$(GOLANG_IMAGE) \
--platform=$(PLATFORMS) \
docker/debug

.PHONY: prepare-docker-buildx
prepare-docker-buildx:
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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>
@pavolloffay
Copy link
Member Author

@albertteoh could you please review?

Copy link
Contributor

@albertteoh albertteoh left a 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:
Copy link
Contributor

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.

@pavolloffay pavolloffay merged commit 8d2ac60 into jaegertracing:master Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants