-
Notifications
You must be signed in to change notification settings - Fork 24
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
NETOBSERV-847 Operator multi-arch builds (upstream) #285
Conversation
Makefile
Outdated
image-push: ## Push OCI image with the manager. | ||
ifneq (,$(findstring quay.io/netobserv/,$(IMG))) | ||
$(error Do not push to quay.io/netobserv) | ||
else | ||
$(OCI_BIN) push ${IMG} | ||
endif | ||
|
||
single-multiarch-linux-build/%: | ||
#The --load option is ignored by podman but required for docker | ||
DOCKER_BUILDKIT=1 $(OCI_BIN) buildx build --load --build-arg TARGETPLATFORM=linux/$* --build-arg TARGETARCH=$* --build-arg BUILDPLATFORM=linux/amd64 -t ${IMG}-$* -f Dockerfile . |
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 don't understand why u set --build-arg BUILDPLATFORM=linux/amd64
while set TARGETPLATFORM=linux/$*
?
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.
BUILDPLATFORM is the platform used to build, se here we build on amd64 while TARGETPLATFORM is the platform where we want the image to run on.
Here, we are cross building on BUILDPLATFORM an image that is intended to run on TARGETPLATFORM.
Makefile
Outdated
|
||
.PHONY: multiarch-manifest-build | ||
multiarch-manifest-build: single-multiarch-linux-push/amd64 single-multiarch-linux-push/arm64 single-multiarch-linux-push/ppc64le | ||
DOCKER_BUILDKIT=1 $(OCI_BIN) manifest create ${IMG} --amend ${IMG}-amd64 --amend ${IMG}-arm64 --amend ${IMG}-ppc64le |
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.
how this change BUILDPLATFORM=linux/amd64
to ppc64le maybe u should change the above to BUILDPLATFORM=linux/$*
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.
See my command above, we never change BUILDPLATFORM because we are always building on amd64, but we change TARGETPLATFORM which is the platform for which is image are built for.
Makefile
Outdated
image-push: ## Push OCI image with the manager. | ||
ifneq (,$(findstring quay.io/netobserv/,$(IMG))) | ||
$(error Do not push to quay.io/netobserv) | ||
else | ||
$(OCI_BIN) push ${IMG} | ||
endif | ||
|
||
single-multiarch-linux-build/%: |
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.
pls add to all new target ## comment style so they show in make help
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.
Sure, following your comment, I have refactored all the Makefiles and updated these docs 😄
Should we add labels as described here ? Supported Labels
Multiple Architectures
|
Yes that will go to CSV right ? |
why we didn't pass in the new build-arg |
Codecov Report
@@ Coverage Diff @@
## main #285 +/- ##
=======================================
Coverage 50.90% 50.90%
=======================================
Files 43 43
Lines 5080 5080
=======================================
Hits 2586 2586
Misses 2293 2293
Partials 201 201
Flags with carried forward coverage won't be shown. Click here to find out more. |
6085580
to
596afab
Compare
Rebased with #300 @jotak @OlivierCazade At the end, it forces me to separate build / push jobs steps and I feel it was better using all at once to ensure arguments are correct. I wonder if there is a smart way to mock |
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.
LGTM
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpinsonneau 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 |
Implements multi arch builds
Please use netobserv/flowlogs-pipeline#426 for main discussion and this PR only for specific items