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

ci: build & publish linux/arm64 Docker images #1367

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

derhuerst
Copy link

On an ARM64 system, I would like to use native Docker images in order not to run an emulation layer.

This PR extends the Docker build & publishing CI workflow to also build linux/arm64 images in addition to linux/amd64 images (implicit default).

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@welcome
Copy link

welcome bot commented Mar 30, 2023

Thanks for opening this pull request! You're awesome. We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of titles with semantic prefixes:

  • fix: Bug with ssl network connections + Java module permissions.
  • `feat: Initial support for multiple @PrimaryKey annotations.
  • docs: update RELEASE.md with new process
    To get this PR to the finish line, please do the following:
  • Read our Contribution Guidelines
  • Follow Google Java style coding standards
  • Include tests when adding/changing behavior
  • Include screenshots

@CLAassistant
Copy link

CLAassistant commented Mar 30, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ davidgamez
❌ derhuerst
You have signed the CLA already but the status is still pending? Let us recheck it.

@derhuerst
Copy link
Author

I think it would make sense to temporarily run the Docker build in this PR too, without publishing of course. What do you think?

@bdferris-v2
Copy link
Collaborator

@derhuerst do you still want to get this checked in? You'll need to sign the CLA before we can do so.

Regarding running the Docker build against this PR. I'll admit I'm not totally clear on how to do that, since the Docker workflow doesn't run for PRs by default. That said, this change seems mostly reasonable, so we could probably check it in as-is and rollback if it causes some issue with the Docker build? Or did you have a different concern in mind?

@derhuerst derhuerst changed the title CI: build & publish linux/arm64 Docker images ci: build & publish linux/arm64 Docker images Apr 19, 2023
branches:
- master
# todo: revert
- docker-arm64
Copy link
Author

Choose a reason for hiding this comment

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

This change should be reverted before merging.

@@ -132,7 +136,9 @@ jobs:
file: ./Dockerfile
build-args: |
VERSION_TAG=${{ steps.prep.outputs.version }}
push: true
# todo: revert
push: false
Copy link
Author

Choose a reason for hiding this comment

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

This change should be reverted before merging.

@bdferris-v2
Copy link
Collaborator

Am I reading correctly that this failed to run on your repo per https://github.com/derhuerst/gtfs-validator/actions/runs/4741545166/jobs/8418790748 ? Any thoughts?

@derhuerst
Copy link
Author

derhuerst commented Apr 19, 2023

Not sure 🤷

@bdferris-v2
Copy link
Collaborator

There is a long running bug with Docker (docker/buildx#59) where multi-platform builds don't work with a load, only a push. I think you can see that in the error from your Docker run:

ERROR: docker exporter does not currently support exporting manifest lists

I think the solution here would be to only build a single platform for the test Docker build but then enable multi platforms for the push. Thoughts?

@derhuerst
Copy link
Author

I think the solution here would be to only build a single platform for the test Docker build but then enable multi platforms for the push. Thoughts?

Sounds good, as I'm mainly interested in having a native Docker image to pull and use.

@davidgamez
Copy link
Member

@derhuerst I created a draft PR with an alternative solution based on @bdferris-v2 comment. Can I get some feedback on #1528 ? Thanks!

@@ -108,6 +111,7 @@ jobs:
build-args: |
VERSION_TAG=${{ steps.prep.outputs.version }}
load: true
platforms: linux/amd64,linux/arm64
Copy link

Choose a reason for hiding this comment

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

To avoid getting

ERROR: docker exporter does not currently support exporting manifest lists

you'll have to either delete this line, or create a third copy of the docker/build-push-action@v3, (one with load, one with platforms, one with push).

@jcfj
Copy link

jcfj commented Jul 19, 2023

Btw, why not set FROM --platform=$BUILDPLATFORM gradle:7-jdk11-alpine AS build in the Dockerfile so the actual building only has to be done once.

For reference, I ended up just rolling with

FROM eclipse-temurin:17-jdk-jammy as valid-gtfs
COPY --from=ghcr.io/mobilitydata/gtfs-validator:4.1.0 gtfs-validator-cli.jar /
WORKDIR /work
COPY foo-gtfs.zip .
RUN java -jar /gtfs-validator-cli.jar -i /work/foo-gtfs.zip -o /work/out/

for running on ARM.

Side note: The Dockerfile currently uses FROM openjdk, which is deprecated.

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.

5 participants