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

fix: add buildx context setup before starting build #1553

Merged
merged 2 commits into from
Nov 21, 2022
Merged

fix: add buildx context setup before starting build #1553

merged 2 commits into from
Nov 21, 2022

Conversation

Xunzhuo
Copy link
Member

@Xunzhuo Xunzhuo commented Nov 21, 2022

Signed-off-by: bitliu [email protected]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1545

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 21, 2022
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 21, 2022
@Xunzhuo
Copy link
Member Author

Xunzhuo commented Nov 21, 2022

Tested and passed:

image

@Xunzhuo Xunzhuo mentioned this pull request Nov 21, 2022
Closed
@Xunzhuo
Copy link
Member Author

Xunzhuo commented Nov 21, 2022

cc @wilsonwu

shaneutt
shaneutt reviewed Nov 21, 2022
View reviewed changes
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Thank you for your continued help through this trial and error process of getting this fixed!

I have a couple comments about leaving behind some comments to document what we've added.

Makefile
.PHONY: image.multiarch.setup
image.multiarch.setup: image.buildx.verify
@if [ "$(shell uname)" == "Linux" ]; then \
docker run --rm --privileged multiarch/qemu-user-static --reset -p yes; \
Copy link
Member

Choose a reason for hiding this comment

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

For future readers it could be helpful to add some comments about this line: firstly what it does and why its needed, and secondly a helpful link to the source of the image.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Makefile
Comment on lines +143 to +144
docker buildx rm $(BUILDX_CONTEXT) || :
docker buildx create --use --name $(BUILDX_CONTEXT) --platform "${BUILDX_PLATFORMS}"
Copy link
Member

Choose a reason for hiding this comment

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

Comments here explaining what these are doing could go a long way for future code readers as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@Xunzhuo
docs: adds comments
b542df8 
Signed-off-by: bitliu <[email protected]>
robscott
robscott reviewed Nov 21, 2022
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @Xunzhuo!

I've got one tiny nit, but going to merge now and leave that for a follow up just to confirm this works.

/lgtm
/approve

Makefile
Comment on lines +133 to +134
BUILDX_CONTEXT = gateway-api-builder
BUILDX_PLATFORMS = linux/amd64,linux/arm64
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: All other environment variables are defined with comments at the top of this file. I'd recommend moving these to the top as well and adding comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I will update it later.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, Xunzhuo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2022
@k8s-ci-robot k8s-ci-robot merged commit 7238f22 into kubernetes-sigs:main Nov 21, 2022
@wilsonwu wilsonwu mentioned this pull request Dec 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaneutt shaneutt shaneutt left review comments

@robscott robscott robscott left review comments

@howardjohn howardjohn Awaiting requested review from howardjohn

Assignees

@robscott robscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Job post-gateway-api-push-images started failing on 09-11
4 participants
@Xunzhuo @k8s-ci-robot @robscott @shaneutt