-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: multi-arch support with buildx in all Dockerfiles #5273
Conversation
✅ Deploy Preview for knative ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Welcome @kahirokunn! It looks like this is your first PR to knative/docs 🎉 |
@knative/serving-reviewers |
# Push the container to docker registry. | ||
docker push "{username}/grpc-ping-go" | ||
# Build and push the container on your local machine. | ||
docker buildx build --platform linux/arm64,linux/amd64 -t "{username}/grpc-ping-go" --push . |
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.
The new command failed for me with the following output:
[+] Building 0.0s (0/0)
ERROR: multiple platforms feature is currently not supported for docker driver.
Please switch to a different driver (eg. "docker buildx create --use")
whereas the original commands worked without issue. If we're going to update it to use buildx
, we'll need some more set up info since it doesn't work out of the box.
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 think adding a link to the buildx should be enough.
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.
is like 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.
If correct, consolidate into one commit.
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.
Personally, I think I'd rather leave the default as-is and provide a link to buildx for those who need to do a multiarch build (which won't be everyone)
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.
If you let people use buildx from the beginning, the container image will run on many people's PCs.
You will not have to worry about CPU architecture errors.
Therefore, buildx is also a good choice.
This Pull Request is stale because it has been open for 90 days with |
keep |
The Dockerfile needs to be updated to support multi arch first ie. |
@kahirokunn gentle ping, are you still planning to update it? |
I will test the updates thanks @kahirokunn. We do not want this for the rest of the samples? I think we need proper image build instructions for all no? |
Do you wish to buildx all images in this repository? |
It would be nice to have build options or maybe a separate page for building alternatives instead of copying the same command everywhere. |
I think I'm still slightly in favor of leaving things as is, with maybe a link out to buildx and/or other alternatives (with the view towards minimizing changes as these samples aren't really actively maintained). But, that said, I don't feel particularly strongly about it (I also don't use a Mac), so if we do decide we should update, that's fine too. But if we do update, we should update all, I think. |
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
@psschwei Okay, I will update all. |
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
Signed-off-by: kahirokunn <[email protected]>
done. how about now? |
@psschwei I would appreciate a review before conflicts arise. thx. |
Sorry about that, I missed the notification for this one one... /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kahirokunn, psschwei 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 |
Proposed Changes
I deployed a docker image built on M1 mac to an intel cluster and got a cpu architecture error.
Now that support for both amd and arm is expanding, I think it is better for users to use buildx to build both to try out the sample.