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.

Sign up for GitHub

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

Jump to bottom

feat(ci): Build multi-arch Docker images (amd64, arm64) #496

Merged
merged 17 commits into from Apr 28, 2023
Merged

feat(ci): Build multi-arch Docker images (amd64, arm64) #496

merged 17 commits into from Apr 28, 2023

Conversation

smiller171
Copy link
Contributor

@smiller171 smiller171 commented Feb 21, 2023 •
edited

Just looking to add ARM builds for my needs

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

Add ARM builds for running on M1/M2 Macs and other Arm CPUs

How can we test changes

After building, run the following command and check that the output contains amd64, arm and arm64

docker manifest inspect ghcr.io/antonbabenko/pre-commit-terraform:nightly | jq -r '.manifests[].platform.architecture' | sort -u

@smiller171
build ARM images
9ed9fdc 
Just looking to add ARM builds for my needs
@MaxymVlasov
Copy link
Collaborator

Are you sure that this is all, which need to make the amd64 packages happily work with arm64? IE

pre-commit-terraform/Dockerfile

Line 75 in 1e623e8

[ "$INFRACOST_VERSION" = "latest" ] && curl -L "$(curl -s ${INFRACOST_RELEASES}/latest | grep -o -E -m 1 "https://.+?-linux-amd64.tar.gz")" > infracost.tgz \

@smiller171
Copy link
Contributor Author

Nope. I hadn't checked that far yet. Give me a bit and I can make it work though.

@MaxymVlasov MaxymVlasov marked this pull request as draft February 22, 2023 12:12
@smiller171 smiller171 marked this pull request as ready for review February 22, 2023 19:25
@smiller171 smiller171 changed the title build ARM images ci: build ARM images Feb 22, 2023
@smiller171 smiller171 changed the title ci: build ARM images ci: Build ARM images Feb 22, 2023
@smiller171
Copy link
Contributor Author

@MaxymVlasov I think this should actually work now. I tested locally and could build it on both platforms, and I think I got the GH Actions config correct.

@MaxymVlasov
Copy link
Collaborator

MaxymVlasov commented Feb 23, 2023 •
edited

In GHA

buildx failed with: ERROR: multiple platforms feature is currently not supported for docker driver. Please switch to a different driver (eg. "docker buildx create --use")

@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 26, 2023
@MaxymVlasov
Copy link
Collaborator

@smiller171 ?

@github-actions github-actions bot removed the stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 28, 2023
@MaxymVlasov MaxymVlasov mentioned this pull request Apr 18, 2023
Closed
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 28, 2023
@MaxymVlasov MaxymVlasov removed the stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 28, 2023
@MaxymVlasov MaxymVlasov changed the title ci: Build ARM images feat(ci): Build multi-arch Docker images (amd64, arm64) Apr 28, 2023
yermulnik
yermulnik reviewed Apr 28, 2023
View reviewed changes
.github/workflows/build-image-test.yaml Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
MaxymVlasov
MaxymVlasov reviewed Apr 28, 2023
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
MaxymVlasov
MaxymVlasov reviewed Apr 28, 2023
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
yermulnik
yermulnik reviewed Apr 28, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md
@@ -97,7 +97,10 @@ All available tags [here](https://github.com/antonbabenko/pre-commit-terraform/p

**Build from scratch**:

When `--build-arg` is not specified, the latest version of `pre-commit` and `terraform` will be only installed.
> **Note**: To build image you need have [`docker buildx`](https://docs.docker.com/build/install-buildx/) enabled as default builder.
> Otherwise - provide `TARGETOS` and `TARGETARCH` as additional `--build-arg`'s to `docker build`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still don't get why don't we just set default value in Dockerfile =) This shouldn't prevent buildx from overriding them. Or am I getting this incorrectly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

buildx is required for multi-arch builds. I'd like to force users to use buildx when they build images from scratch, and because it's not our "core functional" there is no reason to maintain compatibility with legacy docker build

Copy link
Collaborator

Choose a reason for hiding this comment

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

Buildx exist at least for 3 years

Copy link
Collaborator

@yermulnik yermulnik Apr 28, 2023 •
edited

Choose a reason for hiding this comment

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

buildx is required for multi-arch builds. I'd like to force users to use buildx when they build images from scratch, and because it's not our "core functional" there is no reason to maintain compatibility with legacy docker build

Makes sense.

README.md Outdated Show resolved Hide resolved
MaxymVlasov and others added 2 commits April 28, 2023 19:39
@MaxymVlasov @yermulnik
Update README.md
fec6993 
Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>
@MaxymVlasov @yermulnik
Update README.md
79e7fcb 
Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>
@MaxymVlasov MaxymVlasov merged commit 923c2c6 into antonbabenko:master Apr 28, 2023
5 checks passed
antonbabenko pushed a commit that referenced this pull request Apr 28, 2023
@semantic-release-bot
chore(release): version 1.78.0 [skip ci]
c93cb4c 
# [1.78.0](v1.77.4...v1.78.0) (2023-04-28)

### Features

* **ci:** Build multi-arch Docker images (`amd64`, `arm64`) ([#496](#496)) ([923c2c6](923c2c6))
@antonbabenko
Copy link
Owner

This PR is included in version 1.78.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yermulnik yermulnik yermulnik approved these changes

@MaxymVlasov MaxymVlasov MaxymVlasov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@smiller171 @MaxymVlasov @antonbabenko @yermulnik