-
Notifications
You must be signed in to change notification settings - Fork 706
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
Add support for setting target architecture for images #5209
Add support for setting target architecture for images #5209
Conversation
✅ Deploy Preview for kubeapps-dev canceled.Built without sensitive environment variables
|
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 the comment about generalizing the target arch and adding a comment. Otherwise, +1
@@ -27,10 +28,10 @@ all: kubeapps/dashboard kubeapps/apprepository-controller kubeapps/kubeops kubea | |||
# Currently the go projects include the whole repository as the docker context | |||
# only because the shared pkg/ directories? | |||
kubeapps/%: | |||
DOCKER_BUILDKIT=1 docker build -t kubeapps/$*$(IMG_MODIFIER):$(IMAGE_TAG) --build-arg "VERSION=${VERSION}" -f cmd/$*/Dockerfile . | |||
DOCKER_BUILDKIT=1 docker build --platform "linux/$(TARGET_ARCHITECTURE)" -t kubeapps/$*$(IMG_MODIFIER):$(IMAGE_TAG) --build-arg "VERSION=${VERSION}" -f cmd/$*/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'd do --platform "$(TARGET_ARCHITECTURE)"
instead. If Windows containers are ever supported in buildkit, we'd like to also have them.
Besides, I'd rather add some notes commenting the following:
The target platforms supported by buildkit are linux/amd64, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/arm/v7, linux/arm/v6
However, if a user happens to use Windows Containers (not WSL or VM), it won't work as buildkit doesn't support Windows (see microsoft/Windows-Containers#34)
Note that it refers to the container architecture. For instance, a Docker for Windows or Docker for Mac usually runs Linux containers,so we only need to build these two target platforms: linux/amd64
and linux/arm64
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.
After discussing this topic, we've agreed on just providing the TARGET_ARCHITECTURE
and keeping hardcoded the operating system, as we won't likely support Windows containers in the near future and have to provide the whole platform string (eg. linux/arm64
) would add more complexity.
In addition, instead of adding a not regarding the supported platforms, I've decided to add a validation in the code, so Make
will fail with a descriptive error if an unsupported architecture is provided.
40b142f
to
b553849
Compare
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.
Excellent, thanks for the improvement!
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
b553849
to
06929cb
Compare
Description of the change
This PR adds support for specifying the target CPU architecture for the docker images through a parameter in the call to the corresponding make target. This PR doesn't modify the CI, it just adds support for building the docker images for different platforms through
Make
.Benefits
Docker images can be built for different CPU architectures (others than AMD64), so developers using computers with those architectures can take advantage of it.
Possible drawbacks
N/A
Applicable issues
Additional information
N/A