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

Reusable workflow to build and optionally publish docker images #2

Merged
merged 10 commits into from
Feb 14, 2022

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Feb 10, 2022

Cribbed from Synapse's config.

After some back-and-forth, I decided to always build amd64 and arm64 to keep things simple.

If I'm following the recommended branch and tag system recommended for github actions, then I think I need to

  • merge this to main
  • fast forward releases/v1 to the merged commit
  • force push the v1 tag to the merged commit too

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest of it seems good to me

Comment on lines 78 to 95
- name: Build and push amd64
uses: docker/build-push-action@v2
with:
push: ${{ inputs.publish }}
labels: "gitsha1=${{ github.sha }}"
tags: "matrixdotorg/${{ inputs.image-name }}:${{ steps.set-tag.outputs.tag }}"
file: ${{ inputs.dockerfile }}
platforms: linux/amd64

- name: Build and push arm64
if: ${{ inputs.arm64 }}
uses: docker/build-push-action@v2
with:
push: ${{ inputs.publish }}
labels: "gitsha1=${{ github.sha }}"
tags: "matrixdotorg/${{ inputs.image-name }}:${{ steps.set-tag.outputs.tag }}"
file: ${{ inputs.dockerfile }}
platforms: linux/arm64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried this? I'm not 100% sure, but I think this means the ARM64 image will overwrite the tag for the AMD64 image.

Why not accept the list of platforms as an input? TBH though, I'd be happy to always produce the ARM64 image. It's gaining popularity, it's so easy for us to do, but so annoying for users of the packages when it's missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just caution. I don't want to give the impression that we support running all of our stuff on ARM unless we're all in agreement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried this?

Thanks for the pointer to allow a custom name. It works: https://hub.docker.com/r/dmrobertsonelement/sydent

(though it'd be snappier if there were more arm wheels)

.github/workflows/docker.yml Outdated Show resolved Hide resolved
@DMRobertson
Copy link
Contributor Author

Testing this at matrix-org/sydent#492, which is now unhappy on the arm build.

assuming utf-8
This matches the spelling of the org secrets
@DMRobertson
Copy link
Contributor Author

DMRobertson commented Feb 14, 2022

I think @reivilibre is out today. Would you mind taking a quick look at this @squahtx?

Edit: (given the context on the sydent issue above)

.github/workflows/docker.yml Outdated Show resolved Hide resolved
.github/workflows/docker.yml Outdated Show resolved Hide resolved
@squahtx
Copy link
Contributor

squahtx commented Feb 14, 2022

Looks good minus two questions.
I only reviewed from @reivilibre's review onwards.

David Robertson and others added 2 commits February 14, 2022 14:48
Thanks Sean

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>
@DMRobertson DMRobertson dismissed reivilibre’s stale review February 14, 2022 17:48

always arm. Let's test it.

@DMRobertson DMRobertson merged commit 7dc1a35 into release/v1 Feb 14, 2022
@DMRobertson DMRobertson deleted the dmr/docker branch August 1, 2022 10:12
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.

3 participants