Build multiarch protobuf Docker images#73
Build multiarch protobuf Docker images#73yvrhdn wants to merge 2 commits intoopen-telemetry:mainfrom yvrhdn:multiarch
Conversation
| username: ${{ secrets.DOCKER_USERNAME }} | ||
| password: ${{ secrets.DOCKER_TOKEN }} | ||
|
|
||
| - name: Determine tag |
There was a problem hiding this comment.
We might be able to replace this with docker/metadata-action. Basic seems to do what we want here: https://github.com/docker/metadata-action#basic
| builder: ${{ steps.buildx.outputs.name }} | ||
| context: ./protobuf/ | ||
| platforms: linux/amd64,linux/arm64 | ||
| push: ${{ github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/') }} |
There was a problem hiding this comment.
This will always build the multiarch images, but only push if we are on main or building from a tag. Since this action only runs from main or a tag, this isn't necessary I think? Anyway, I copied the original logic for now.
|
I see I used the wrong secret name, this should work 🙂 |
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
The dockerfile changes look good, I submitted a similar PR without noticing this one existed before https://github.com/open-telemetry/build-tools/pull/75/files.
Regarding the workflow changes, it looks fine, but maybe @jpkrohling has some guidance around the preferred tooling for publishing docker images.
| unzip -q /protoc-gen-lint_linux_amd64.zip && \ | ||
| install -Ds /protoc-gen-lint-out/protoc-gen-lint /out/usr/bin/protoc-gen-lint | ||
| unzip -q /protoc-gen-lint_linux_${TARGETARCH}.zip && \ | ||
| install -D /protoc-gen-lint-out/protoc-gen-lint /out/usr/bin/protoc-gen-lint |
There was a problem hiding this comment.
Is it necessary to remove the -s flag?
jpkrohling
left a comment
jpkrohling
left a comment
There was a problem hiding this comment.
LGTM. I don't quite like the tagging logic but might be OK if the linked action (docker/metadata-action) isn't suitable. In general, this is a similar approach we use in other repositories (Jaeger and some operators, IIRC) with the exception of the collector-releases and -builder, but we don't produce container images for every commit there. Instead, we produce the artifacts, including container images, via goreleaser only during releases.
| ARG UPX_VERSION | ||
| RUN mkdir -p /upx && curl -sSL https://github.com/upx/upx/releases/download/v${UPX_VERSION}/upx-${UPX_VERSION}-amd64_linux.tar.xz | tar xJ --strip 1 -C /upx && \ | ||
| ARG TARGETARCH | ||
| RUN mkdir -p /upx && curl -sSL https://github.com/upx/upx/releases/download/v${UPX_VERSION}/upx-${UPX_VERSION}-${TARGETARCH}_linux.tar.xz | tar xJ --strip 1 -C /upx && \ |
There was a problem hiding this comment.
You are only building linux/amd64,linux/arm64, so, it should be fine, but I had problems before with binaries for macOS after going through upx. Do you know how big the image is before and after upx? Perhaps it's not worth the risk?
|
@kvrhdn please address all the concerns/comments. |
4 participants
This adapts
protobuf/Dockerfileso it can be built for amd64 and arm64 targets by usingTARGETARCH.I had to update
PROTOC_GEN_LINT_VERSIONas only v0.2.4 has downloads for all archs.Next I updated the GitHub Actions workflow, I'm using docker/build-push-action to build multiarch images. Under the hood this uses docker buildx.
You can run buildx locally using:
Fixes #42