Added support for multi-arch i.e. s390x & pp64cle #62
Conversation
Remoe shell as it can't be used with github action docker/build-push-action
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: modassarrana89 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 |
There was a problem hiding this comment.
Thank you @modassarrana89 for this PR !
Some very early comments:
- can you elaborate on why you are suggesting this approach in place of
scripts/build_deploy.shwhich was used at present, please? - would it be possible to include ARM, since this multi-arch build is being performed?
thank you as this improves on:
/ok-to-test
ps: also @modassarrana89 kindly sign-off commits to comply with DCO, KF community has decided it's being used going forward. thanks!
Dockerfile
Outdated
| # Download protoc compiler v24.3 | ||
| RUN set -ex\ | ||
| ; ARCH=$(uname -m) ; echo $ARCH \ | ||
| ; if [ "$ARCH" == "s390x" ] ; then ARCH="s390_64" ; elif [ "$ARCH" == "ppc64le" ] ; then ARCH="ppcle_64" ; fi \ | ||
| ; wget -q https://github.com/protocolbuffers/protobuf/releases/download/v24.3/protoc-24.3-linux-$ARCH.zip -O protoc.zip \ | ||
| ; unzip -q protoc.zip \ | ||
| ; bin/protoc --version \ | ||
| ; rm protoc.zip \ | ||
| ; |
There was a problem hiding this comment.
The download of protoc is already performed as part of make deps which includes make bin/protoc, in line below 34.
This was improved with #52
Kindly consider extending scripts/install_protoc.sh for your platforms, so we don't need this extra Docker RUN ?
There was a problem hiding this comment.
i didn't know about the protoc change . Once it is approved , i will update my changes accordingly for protoc
There was a problem hiding this comment.
Once it is approved , i will update my changes accordingly for protoc
Hi, I cannot approve until the setup can be replicated and sign-off commits to comply with DCO as required by KF community.
It seems to me at least the following modifications are needed:
diff --git a/Dockerfile b/Dockerfile
index 5980940..d777709 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -19,20 +19,6 @@ RUN yum install -y nodejs npm java-11
# Copy the go source
COPY ["Makefile", "main.go", ".openapi-generator-ignore", "openapitools.json", "./"]
-
-# Download protoc compiler v24.3
-RUN set -ex\
- ; ARCH=$(uname -m) ; echo $ARCH \
- ; if [ "$ARCH" == "s390x" ] ; then ARCH="s390_64" ; elif [ "$ARCH" == "ppc64le" ] ; then ARCH="ppcle_64" ; fi \
- ; wget -q https://github.com/protocolbuffers/protobuf/releases/download/v24.3/protoc-24.3-linux-$ARCH.zip -O protoc.zip \
- ; unzip -q protoc.zip \
- ; bin/protoc --version \
- ; rm protoc.zip \
- ;
-
-# Download tools
-RUN make deps
-
# Copy rest of the source
COPY .git/ .git/
COPY cmd/ cmd/
diff --git a/Makefile b/Makefile
index a227ee8..bba3fc7 100644
--- a/Makefile
+++ b/Makefile
@@ -225,7 +225,7 @@ PLATFORMS ?= linux/amd64,linux/s390x,linux/ppc64le
.PHONY: image/buildx
image/buildx: ## Build and push docker image 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\}/' ./Dockerfiles/Dockerfile > ./Dockerfiles/Dockerfile.cross
+ sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' ./Dockerfile > ./Dockerfile.cross
- $(DOCKER) buildx create --name project-v3-builder
$(DOCKER) buildx use project-v3-builder
- $(DOCKER) buildx build --push --platform=$(PLATFORMS) --tag ${IMG}:$(IMG_VERSION) -f Dockerfile.cross .
diff --git a/scripts/install_protoc.sh b/scripts/install_protoc.sh
index b7b165e..e346c14 100755
--- a/scripts/install_protoc.sh
+++ b/scripts/install_protoc.sh
@@ -12,6 +12,10 @@ fi
ARCH="x86_64"
if [[ "$(uname -m)" == "arm"* ]]; then
ARCH="aarch_64"
+elif [[ "$(uname -m)" == "s390x" ]]; then
+ ARCH="s390_64"
+elif [[ "$(uname -m)" == "ppc64le" ]]; then
+ ARCH="ppcle_64"
fi
mkdir -p ${SCRIPT_DIR}/../bin
as in:
- keep using
scripts/install_protoc.shfor downloading protoc as part of makefile, not re-download as part of dockerfile - not sure why this original PR changes makefile to use dockerfile in subdirectory
./Dockerfiles/🤔
Also, I believe it would be ideal for makefile image/buildx to not push the images, wdyt?
We appreciate your PR a lot! We just need to account for these comments and kindly sign-off commits so to make it easier to collaborate.
|
@tarilabs I have added architecture changes in install_protoc.sh , removed protoc part from dockerfile , updated Makefile as well. Addressed all your comments . Please review once. |
thank you for the updates @modassarrana89 ! As mentioned last Apr 12, and last Apr 14, ensure you signing-off your commits to comply with DCO, KF community has decided it's being used going forward. This is required. |
Unfortunately i am not able to rebase it properly , as i mentioned before, my initial code changes were 1 week old code. so during rebase , i am getting issues with your fix . Do you mind , if i cancelled this PR & actually create a new one with my changes & proper signoff ( as required ). Please suggest |
not sure which of "my" fix you are referring to, but feel free to close this PR and reopen a new one, it's no problem! thank you |
periodic sync upstream KF to midstream ODH
Added support for multi-arch i.e. s390x & pp64cle
Description
How Has This Been Tested?