-
Notifications
You must be signed in to change notification settings - Fork 768
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
Add support for multiarch images, include ARM64 #1453
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Lee Ween Jiann <[email protected]>
Hope you can help review |
Hi @lwj5 , thanks for the PR. I will try to get to it in next few days. |
Any updates or feedback would be good |
cache-from: type=gha | ||
cache-to: type=gha,mode=max |
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.
What is the purpose of this?
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.
This exports and retrieves docker build cache to GitHub cache to speed up build time if certain layers have not changed.
https://docs.docker.com/reference/cli/docker/buildx/build/#cache-to
on your local host, this is done transparently. Hence why the 2nd docker build is slightly faster
FROM golang:1.21 as builder | ||
FROM --platform=$BUILDPLATFORM golang:1.21 as builder | ||
|
||
ARG TARGETOS TARGETARCH |
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.
These variables are not set anywhere. What are their default values? Are these specific to Docker? Can we remove them?
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.
These arg are auto populated if not specified. They default to the local platform.
https://docs.docker.com/reference/dockerfile/#automatic-platform-args-in-the-global-scope
They are required for the multi arch build used in the go build line
@@ -1,5 +1,7 @@ | |||
# Build the manager binary | |||
FROM golang:1.21 as builder | |||
FROM --platform=$BUILDPLATFORM golang:1.21 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.
I assume $BUILDPLATFORM
will default to the arch on which the build is running on if not set. Can you confirm if make docker-build
runs fine and local dev builds are not impacted with this change?
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.
Yes this will default to the local platform it is running on.
There is no difference to local builds. They work as-is like previously
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.
This is a feature I look forward to being incorporated into the argocd-operator!
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
Support for building ARM64 images
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
How to test changes / Special notes to the reviewer:
I see a set up go and go cache here, but doesn't seem to be used
argocd-operator/.github/workflows/publish.yaml
Line 22 in e5872c5
If you would like, E2E tests can be added for macOS runner to test arm64