-
Notifications
You must be signed in to change notification settings - Fork 33
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 release workflow #12
Conversation
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
1f3f015
to
edbd1af
Compare
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
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.
@aliariff Thanks for putting this together. Some questions and comments below.
Makefile
Outdated
push: images ## Push multi arch docker images to the registry | ||
docker buildx build \ | ||
--platform $(SUPPORTED_ARCHS) \ | ||
--output "type=image,push=true" \ |
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.
TIOLI: consider using --output "type=registry"
or just --push
. See https://github.com/docker/buildx#registry.
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.
Possible, but this one just to make a clear distinction between build without pushing and build with pushing.
Makefile
Outdated
docker buildx build \ | ||
--platform $(SUPPORTED_ARCHS) \ | ||
--output "type=image,push=false" \ | ||
--tag $(REPO):$(VERSION) \ | ||
--tag $(REPO):latest \ |
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 think this only works OOB with Docker Desktop, right? Specifically, it only works with a Buildx builder that uses the docker-container
driver. On my Ubuntu laptop though, the default builder uses the docker
driver, which doesn't support multi-platform builds.
Assuming that we want the Makefile to work outside of Docker Desktop and CI too, we should add some instructions like the following to the README:
On some environments where the default Buildx builder doesn't use the docker-container
driver, the make images
target may return the following error:
$ make images
multiple platforms feature is currently not supported for docker driver. Please switch to a different driver (eg. "docker buildx create --use")
Makefile:57: recipe for target 'images' failed
make: *** [images] Error 1
Use the following commands to create a builder that uses the docker-container
driver:
$ docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
$ docker buildx create --name=multiarch --driver=docker-container --platform=linux/amd64,linux/arm64,linux/arm/v7 --use
multiarch
$ docker buildx inspect multiarch --bootstrap
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 agree, instructions in the readme or directly in make help
about how to build multi-arch images will be helpful.
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.
Maybe we can script it as a different make target (say, make builder
), and mention in the README that if one encounters the above error message, run make builder
. What do you think of something like:
.PHONY: builder
builder:
docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
docker buildx create --name=multiarch-builder --driver=docker-container --platform="${SUPPORTED_ARCHS}" --use
multiarch
docker buildx inspect multiarch-builder --bootstrap
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.
That is also possible, I will try it out.
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.
@ihcsim
is it necessary to run docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
?
because in this case, we don't use QEMU, we just utilize golang internal cross-compile features.
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.
AIUI, if the Dockerfile has the RUN
commands, we will need the qemu in order for those commands to be executed during the multi-arch build.
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.
AFAICT, the buildx github action has to do the same thing. See https://github.com/crazy-max/ghaction-docker-buildx/blob/44fd46c7833e095f13bb1d8ace66bd821198bfef/src/main.ts#L24-L38.
Makefile
Outdated
. | ||
|
||
.PHONY: push | ||
push: images ## Push multi arch docker images to the registry |
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 will cause CI to run docker buildx build
twice, using different --output
configuration. I think on CI, we should be able to just use make push
.
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, but the second run will be using the cache Overall the whole release workflow finishes in 1 minute.
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
README.md
Outdated
Run `make builder` to create Buildx instance before starting to build the images. | ||
|
||
Run `make images` to start build the images. |
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 for adding this section. Can we provide more context around why and when a user will need to run make builder
? How about something like:
In some local environments like Ubuntu, where the default Buildx builder uses the docker
driver, the make images
command might fail with the following error:
$ make images
multiple platforms feature is currently not supported for docker driver. Please switch to a different driver (eg. "docker buildx create --use")
Makefile:57: recipe for target 'images' failed
make: *** [images] Error 1
To fix this, you can create a new Buildx builder instance by running make builder
. This command will create a builder that uses the docker-container
driver that can build multi-platform images. For more information, see the Buildx builder documentation.
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
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.
LGTM.
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.
Very thoughtful work, @aliariff thanks!
.github/workflows/release.yml
Outdated
runs-on: ubuntu-18.04 | ||
steps: | ||
- name: Checkout code | ||
# actions/checkout@v2 |
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.
Is there a reason to leave these comments in?
@cpretzer
Tested manually in my local fork:
https://github.com/aliariff/linkerd2-proxy-init/pull/4/checks?check_run_id=800916941
Using the docker hub registry and my user & pass.
The resulting images are in https://hub.docker.com/r/etni35/proxy-init/tags
This version only can be tested properly when we create a new release tag.