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

fix(Makefile): docker-buildx do not add duplicate --platform #182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(Makefile): docker-buildx do not add duplicate --platform #182

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

While it could be argued that this target does not work with podman, it's from kubebuilder template which also use the same $(CONTAINER_TOOL).

And I am PR'ing into kubebuilder in kubernetes-sigs/kubebuilder#4545

Copy link
Collaborator

Choose a reason for hiding this comment

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

See kubernetes-sigs/kubebuilder#4544 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kaovilai it's your call if we want to add it or not, but it should be commented that it differs from upstream kubebuilder, so we don't simply replace it in the future by following for example instructions like:
https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.34.0/

e.g. Without clear comment to the developer I would replace everything within docker-buildix including diff from upstream kubebuilder.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kaovilai could you please add the comment to the code and I will approve?

Copy link
Member Author

Choose a reason for hiding this comment

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

or do you prefer a third target that would not conflict with kubebuilder scaffold?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kaovilai that is cool idea (third target) - I like it more then comment.

Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ PLATFORMS ?= linux/arm64,linux/amd64,linux/s390x,linux/ppc64le
.PHONY: docker-buildx
docker-buildx: ## Build and push docker image for the manager for cross-platform support
# copy existing Dockerfile and insert --platform=${BUILDPLATFORM} into Dockerfile.cross, and preserve the original Dockerfile
sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross
(cat Dockerfile | grep -qe '--platform' && cp Dockerfile Dockerfile.cross) || sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross
- $(CONTAINER_TOOL) buildx create --name project-v3-builder
$(CONTAINER_TOOL) buildx use project-v3-builder
- $(CONTAINER_TOOL) buildx build --push --platform=$(PLATFORMS) --tag ${IMG} -f Dockerfile.cross .
Expand Down