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

Build Docker image for linux/aarch64 #1206

Merged
merged 5 commits into from
Mar 27, 2022

Conversation

ivy
Copy link
Contributor

@ivy ivy commented Sep 3, 2021

I noticed there's only a Docker image for linux/amd64 and wanted to propose adding support for linux/aarch64. It's pretty useful to me as an Apple M1 and Docker Desktop user since I can get native performance out of tflint and avoid potential hiccups with x86_64 emulation. 😄

Copy link
Member

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

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

Neat, first time seeing this!

https://github.com/docker/build-push-action/blob/v2/docs/advanced/multi-platform.md

I've approved the workflow run, take a look at the failure and my comments, happy to get this merged if you can get it working. FWIW I don't think the performance is much of an issue here (benchmarks appreciated if you've found otherwise) compared to the inconvenience of adding --platform=linux/amd64.

.github/workflows/docker.yml Outdated Show resolved Hide resolved
.github/workflows/docker.yml Outdated Show resolved Hide resolved
@@ -30,8 +42,9 @@ jobs:
- name: Build and push
uses: docker/build-push-action@v2
with:
platforms: ${{ matrix.platform }}
Copy link
Member

Choose a reason for hiding this comment

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

Is this right to be doing a matrix build/push for each platform? Do new platforms push into existing images?

If not, remove the matrix from this job and add a platforms output. Then, split docker run out into a test job. That job will be a matrix job, using the platforms output from the build job to set the platform list for the matrix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. After looking more closely with docker image inspect, this might not be working the way I'd assumed. 😅

Ivy Evans added 4 commits September 3, 2021 12:50
While `aarch64` is an alias, the fully expanded identifier is more
explicit.
The `load` option is necessary for Docker Buildx otherwise built images
are only accessible to the builder.
Unfortunately, the previous job matrix will overwrite the other
platforms so instead, we make use of an environment variable to specify
each platform and pass it as-is when interfacing with  Docker Buildx.
@ivy
Copy link
Contributor Author

ivy commented Sep 3, 2021

@bendrucker Thanks for your feedback! Unfortunately, it's a bit difficult to test these GitHub Actions myself but I think it should work.

FWIW I don't think the performance is much of an issue here

Definitely! In this particular project's case, the performance difference is negligible. If you're interested in offering native support like the rest of the binary releases, I'd still consider it valuable. I'd understand though if it wasn't something you or other maintainers wanted to worry about. 🤷‍♀️

@bendrucker
Copy link
Member

Seems like Actions doesn't consider you a "contributor" until a commit is associated with you in the repository's default branch. I thought workflow run approvals applied on a user's first PR/workflow run for a repository, but I guess we have to approve each run.

https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/

@bendrucker
Copy link
Member

Speaking of performance...

The arm64 build is ~5x slower than amd64. To be expected I guess, goreleaser takes ~20 mins to cross-compile and upload release binaries. Not really an issue considering we're also building for Windows.

necessary to "re-run" the build (everything's already cached) to load

Doesn't seem that way (cached), the Test step is taking as long as the previous build.

- name: docker run
run: docker run ghcr.io/${{ github.repository }}:${{ steps.meta.outputs.version }} -v
- name: Test
# NOTE(ivy): Docker Buildx doesn't support the --load flag when multiple
Copy link
Member

Choose a reason for hiding this comment

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

Is there any issue we can link to on this?

@wata727
Copy link
Member

wata727 commented Mar 6, 2022

@ivy What's the status now? I'm interested in merging this feature.

@wata727
Copy link
Member

wata727 commented Mar 27, 2022

I've confirmed the comments and I think this pull request is ready to merge. Revert if there is any problem.
Thank you for your contributions!

@wata727 wata727 merged commit 3c649ce into terraform-linters:master Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants