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

chore: build & publish multi-platform image(linux/arm64, linux/adm64) Docker images #1528

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

Conversation

davidgamez
Copy link
Member

@davidgamez davidgamez commented Jul 5, 2023

Summary:

Alternative solution for #1367. This PR addresses the issue related with no supported multi-platform docker with load parameter (PR comment, docker/buildx#59).

Core changes

  • Replace gradle:7-jdk11-alpine docker builder image with eclipse-temurin:11 as gradle:7-jdk11-alpine don't support arm64 architecture.
  • Create Github matrix to build and test each docker platform
  • Create a separate docker publish step without load parameter to publish a multi-platform docker image

Expected behavior:

A multi-platform docker image is published.

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)

@davidgamez
Copy link
Member Author

I tested in OS and Linux, and I was able to pull the docker image with the right architecture locally. Used:

docker run --rm ghcr.io/mobilitydata/gtfs-validator:latest-test-multiplat --help
docker inspect ghcr.io/mobilitydata/gtfs-validator:latest-test-multiplat

Working Github action example https://github.com/MobilityData/gtfs-validator/actions/runs/5464582100

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

✅ Rule acceptance tests passed.
New Errors: 0 out of 1436 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1436 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1436 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 0 out of 1436 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1436 sources (~0 %) are corrupted.
Commit: f83c557
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

@davidgamez davidgamez marked this pull request as ready for review July 7, 2023 14:03
@github-actions
Copy link
Contributor

✅ Rule acceptance tests passed.
New Errors: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1439 sources (~0 %) are corrupted.
Commit: 8bce645
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

@jcpitre jcpitre self-requested a review August 11, 2023 13:27
fi
echo "Set DOCKER_TAGS=${DOCKER_TAGS}"

echo ::set-output name=version::${AXION_VERSION}
Copy link
Contributor

Choose a reason for hiding this comment

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

::set-output is deprecated.
Here is the suggestion for replacement: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/
But they say they postponed removal because of significant use. So I guess it's up to you to change it or not.

echo ::set-output name=tags::${DOCKER_TAGS}
echo ::set-output name=created::$(date -u +'%Y-%m-%dT%H:%M:%SZ')
- name: Build Docker container image
uses: docker/build-push-action@v3
Copy link
Contributor

@jcpitre jcpitre Aug 11, 2023

Choose a reason for hiding this comment

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

Can you explain why it seems to build an image twice, one here and once in "Push Docker container image" below. The two command lines to build the images are identical (obtained form the logs) except for the --push

runs-on: ubuntu-latest
strategy:
matrix:
platform: [amd64 , arm64]
Copy link
Contributor

@jcpitre jcpitre Aug 14, 2023

Choose a reason for hiding this comment

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

There was a problem running this action (see https://github.com/MobilityData/gtfs-validator/actions/runs/5833255963)
I suspect it's because with the matrix we build 2 images but give them the same tag (ghcr.io/mobilitydata/gtfs-validator:latest, see line 95 below).
Since it appears the matrix strategy runs in parallel for the 2 platforms, this could cause a race condition and it will fail or not depending on the time it takes to create and put the images in the local registry.
I suggest you add a suffix to the DOCKER_TAGS variable on line 95 below. Something like:
DOCKER_TAGS="${DOCKER_IMAGE}.${{ matrix.platform }}:latest"
And see what happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tried it and the problem is still present. Although I think it would not hurt to keep this fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to run the arm64 version on an arm64 platform, or use emulation.

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.

2 participants