-
Notifications
You must be signed in to change notification settings - Fork 0
Multi-arch docker build #28
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
Conversation
0ab370d
to
fbfa94e
Compare
fbfa94e
to
8d747b1
Compare
if-no-files-found: error | ||
retention-days: 1 | ||
|
||
merge-descriptors: |
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.
Why is this named merge-descriptors?
This could use a code comment describing the purpose of the task.
And a description of this part of the change in the commit message would be helpful as well.
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.
+1 I'm similarly confused about the purpose of this job
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 am too, but what I gather is it's a one-time update of the image metadata executed after all platform builds complete. Added a comment.
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.
Major feedback is that some commentary about how this code is derived would greatly help us in the future.
- name: Upload digest | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: digests-${{ env.PLATFORM_PAIR }} |
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.
Would this be simpler?
name: digests-${{ env.PLATFORM_PAIR }} | |
name: digests-${{ matrix.platform }}/- |
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 may be but I don't fully understand the reasons why it is as it is in the Docker documentation. I'm going to leave it assuming echo "PLATFORM_PAIR=${platform//\//-}"
is not equal to appending a -
.
|
||
steps: | ||
- name: Prepare |
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.
See comment below, might not need this:
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.
OK, I see that this is verbatim from the documentation.
I think it would be helpful to link to that documentation in a comment here, and explicitly note in said comment that "These steps are copied verbatim from ...".
- name: Prepare | ||
run: | | ||
platform=${{ matrix.platform }} | ||
echo "PLATFORM_PAIR=${platform//\//-}" >> $GITHUB_ENV |
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 you can just use:
echo "PLATFORM_PAIR=${platform//\//-}" >> $GITHUB_ENV | |
env: | |
- PLATFORM_PAIR: ${{ matrix.platform }}/- |
at point of use
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 is simpler but at the risk of breaking mysterious things that seem to work right now :)
if-no-files-found: error | ||
retention-days: 1 | ||
|
||
merge-descriptors: |
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.
+1 I'm similarly confused about the purpose of this job
- name: Login to Docker Hub | ||
uses: docker/login-action@v3 | ||
with: | ||
username: ${{ secrets.DOCKERHUB_USERNAME }} |
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.
What's the thinking for picking Docker Hub (vs. a Google or AWS repo or both)? Just curiousity.
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.
Just picked it based on it's popularity for hosting publicly available images vs. GCP/AWS more typically used for internal images. But I'm sure you can achieve the same with other solutions.
Build Docker images for amd64 (existed) and arm64 (new).
These changes are based on Docker documentation: https://docs.docker.com/build/ci/github-actions/multi-platform/
This PR was tested by executing the GitHub Action from a PR trigger.
New image arm64 version published to DockerHub:
Also: updated 3rd party GitHub Action versions