-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Makefile: docker-build should use --load for newer docker build aliased to buildx #4547
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @kaovilai. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Hi @kaovilai 👋 Really appreciate you taking the time to improve Kubebuilder! 🚀 I hope you don’t mind, but I’ll close this PR with the following reference: If you disagree and still think we should add this change, Thanks again for your contribution! 😊 |
@@ -183,7 +183,7 @@ run: manifests generate fmt vet ## Run a controller from your host. | |||
# More info: https://docs.docker.com/develop/develop-images/build_enhancements/ | |||
.PHONY: docker-build | |||
docker-build: ## Build docker image with the manager. | |||
$(CONTAINER_TOOL) build -t ${IMG} . | |||
$(CONTAINER_TOOL) build --load -t ${IMG} . |
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.
It is the default docker build and it is working fine. We should not add the flag here.
You probably want to replace the --flag push
with --load
in the docker buildX instead, see: https://github.com/kubernetes-sigs/kubebuilder/pull/4547/files#diff-e5d6d4f9d67174f1f3764674eb786688353c77cef8514e24d1323e55e5a4a7e8R192-R209
However, the WARNING is only faced in the buildx(above target) when it has no flags --load
or --push
You removed it from your own project.
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.
docker buildx build --push
do not require --load
at all.. if you have --push, you can skip --load.
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.
Hi @kaovilai
See: #4546 (comment)
I reproduce your scenario:
You are calling docker buildx
without ANY of the flags.
It is required to have --load or push
And this target here is docker build ( should not use any of those )
You can either validate the behaviour with the samples under testdata to check that all generated by Kubebuilder works fine.
So, I am closing this one as deferred if you do not mind
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.
@camilamacedo86 per #4546 (comment) this can be triggered with just docker-build per my reproduce. I did not modify the default scaffold.
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 one of the PRs that benefits the users of the default scaffold without modification.
Closing this based on the comment : #4546 (comment). We should not do this change. |
Proposal was part of kubernetes-sigs/kubebuilder#4547
…ed to buildx
Fixes #4546
Same as migtools/oadp-non-admin#180
Related: #4545