-
Notifications
You must be signed in to change notification settings - Fork 82
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
feat: linux/arm64 compatibility #319
feat: linux/arm64 compatibility #319
Conversation
c5497e0
to
df06aa6
Compare
df06aa6
to
8a152f3
Compare
Hi @isometry, Thanks for the feature request and the PR! We will review with the team and get back to you soon. |
Thank you. We plan to support ARM this year, however, it doesn't look like we can deliver ARM support in Q2. |
What more is needed beyond the changes in this PR? Can we help? |
I imagine they might want a better integrated solution? Your PR understandably makes efforts to be non-invasive and low risk (by creating a temporary hacked version of the Dockerfile), but IMO a proper solution would just have a working Dockerfile in all cases. In any case @isometry I tried your patch but the arm64 images aren't actually working on arm64. Classic "exec format error" on execution. This is because of docker/buildx#510 and I needed to make this change to the Dockerfile to get a proper build: diff --git a/Dockerfile b/Dockerfile
index 7288a6b..533705f 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -2,8 +2,8 @@
FROM golang:1.21 as builder
WORKDIR /workspace
-ARG TARGETARCH=amd64
-ARG TARGETOS=linux
+ARG TARGETARCH
+ARG TARGETOS
ENV GOPROXY=direct
# Copy the Go Modules manifests
@@ -18,8 +18,8 @@ COPY main.go main.go
COPY pkg/ pkg/
ENV CGO_ENABLED=0
-ENV GOOS=$TARGETOS
-ENV GOARCH=$TARGETARCH
+ENV GOOS=${TARGETOS:-linux}
+ENV GOARCH=${TARGETARCH:-amd64}
ENV GO111MODULE=on |
Thanks @jtackaberry! The Dockerfile hack is what operator-sdk is currently pushing (IIRC). I personally favour building a minimalist image with ko. My initial local patch actually included your fixes but I reverted in a misguided attempt to minimise the footprint of the patch after seeing other operators using the original pattern. Oh well. I'll update :-) |
b5323b5
to
4c452bf
Compare
@jtackaberry : as per your suggestion, I've tidied up to remove unnecessary shenanigans. |
4c452bf
to
54b818f
Compare
@@ -1,9 +1,9 @@ | |||
# Build the manager binary | |||
FROM golang:1.22 as builder | |||
FROM --platform=${BUILDPLATFORM} golang:1.22 as builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"BUILDPLATFORM is part of a set of automatically defined (global scope) build arguments that you can use. It will always match the platform or your current system and the builder will fill in the correct value for us."
from: https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/
Putting this here for reference
Fixes: cert-manager#283 Signed-off-by: Robin Breathe <robin.breathe@nexthink.com> Signed-off-by: Brady Siegel <brsiegel@amazon.com>
54b818f
to
f1a5fd0
Compare
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.