-
Notifications
You must be signed in to change notification settings - Fork 291
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
Adds image automated build #189
Adds image automated build #189
Conversation
/cc @aramase PS: The reason for the build script, is in case you will want to build multi-arch images in the future. See here an example: https://github.com/kubernetes/kubernetes/blob/master/test/images/image-util.sh#L131 |
45a6bd6
to
2981794
Compare
@claudiubelu The e2e tests are failing because they're looking for Dockerfile in the repo root. Once you update that to |
646062b
to
b6df6f7
Compare
I've tried building this image with the added script. The image I've built is at |
b6df6f7
to
7fa01cf
Compare
FWIW, I've updated the way the image is built, so the Windows nodes are not required at all: https://paste.ubuntu.com/p/phqKvJDDbt/ From what I've tried, containers using the image The more difficult part of this was actually tagging the manifest list with the proper I've also updated the image in |
7fa01cf
to
0be100b
Compare
@claudiubelu We recently merged the PR to use
This is great! |
@@ -51,9 +52,15 @@ build: setup | |||
build-windows: setup | |||
CGO_ENABLED=0 GOOS=windows go build -a -ldflags ${LDFLAGS} -o _output/secrets-store-csi.exe ./cmd/secrets-store-csi-driver | |||
image: | |||
docker buildx build --no-cache --build-arg LDFLAGS=${LDFLAGS} -t $(IMAGE_TAG) -f Dockerfile --platform="linux/amd64" --output "type=docker,push=false" . | |||
docker buildx build --no-cache --build-arg LDFLAGS=${LDFLAGS} -t $(IMAGE_TAG) -f docker/Dockerfile --platform="linux/amd64" --output "type=docker,push=false" . |
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.
there is no docker/
dir?
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.
Indeed, I moved some things on rebase (there was a merge conflict). Because of some recent changes, I had to rewrite some bits of the building process, and I moved everything in the root directory.
I still prefer having the docker folder. The reason is because we're configuring the Image Promoter to build the image when changes to files in the docker/.*
are being made (for example, you'd trigger the builder on an image version bump). Having everything on the root directory would mean that the builder would be triggered on every PR, which is unnecessary.
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.
@claudiubelu I'm just wondering if we can make it more architecture natural so that we can build other platforms as well like arm, ppc64le and s390x as well?
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 did, in the build.sh
below.
As I mentioned before, I've made a separate docker/Makefile
which is going to be used by the Image Builder / Promoter. This target is still being used for testing, and is called in this Makefile
. This only builds the linux/amd64
image, which is sufficient for testing, while mine builds all images, which takes a lot more time, especially due to the Windows images.
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.
cool
a64b2d7
to
269fe38
Compare
Hmm, from what I see, there is still only linux/amd64 and windows/amd64. There is no arm, amr64, etc. archs added. |
@@ -51,9 +52,15 @@ build: setup | |||
build-windows: setup | |||
CGO_ENABLED=0 GOOS=windows go build -a -ldflags ${LDFLAGS} -o _output/secrets-store-csi.exe ./cmd/secrets-store-csi-driver | |||
image: | |||
docker buildx build --no-cache --build-arg LDFLAGS=${LDFLAGS} -t $(IMAGE_TAG) -f Dockerfile --platform="linux/amd64" --output "type=docker,push=false" . | |||
docker buildx build --no-cache --build-arg LDFLAGS=${LDFLAGS} -t $(IMAGE_TAG) -f docker/Dockerfile --platform="linux/amd64" --output "type=docker,push=false" . |
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.
@claudiubelu I'm just wondering if we can make it more architecture natural so that we can build other platforms as well like arm, ppc64le and s390x as well?
269fe38
to
1afb7c6
Compare
docker/Makefile
Outdated
|
||
REGISTRY?=docker.io/deislabs | ||
IMAGE_NAME=driver | ||
IMAGE_VERSION?=v0.0.10 |
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.
Could you update this to v0.0.11
as that's the last release we cut?
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.
@claudiubelu The PR looks great! 🎉
Just one small nit to update the IMAGE_VERSION
to the latest release version.
In order for the secret store CSI image to be automatically built and published to the staging registry (from which it will be promoted), the cloudbuild.yaml file has been added. The file was added in conformance with [1]. Adds the docker folder, which contains several items: - cloudbuild.yaml - BASEIMAGE: the base image to use when building an image for a certain os/arch (or os/arch/version for Windows). - BASEIMAGE_CORE: for Windows images, from which image to copy necessary files (DLLs). - Makefile - build.sh: script that can build, push, and create the manifest list. - Dockerfile: needed to build the Linux docker image. Can now accept the a BASEIMAGE arg, which can be useful when building multi-arch images. - windows.Dockerfile: needed to build the Windows docker image. Can now accept BASEIMAGE and BASEIMAGE_CORE args, which can be useful when building images for multiple Windows versions. The image building process will be triggered when changes to the files in the docker changes are made (for example, you bump the image version, so a new image is built). Adds support for multiple Windows versions. The image will be created with the name gcr.io/k8s-staging-csi-secrets-store/driver:VERSION. Bumps the driver's image version. [1] https://github.com/kubernetes/test-infra/blob/master/config/jobs/image-pushing/README.md
Updated version |
1afb7c6
to
a2c63e0
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.
/lgtm
/hold
for @ritazh review
/hold cancel |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aramase, claudiubelu 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 |
In order for the secret store CSI image to be automatically built and published to the staging registry (from which it will be promoted), the
cloudbuild.yaml
file has been added.The file was added in conformance with [1].
Adds the docker folder, which contains several items:
(or os/arch/version for Windows).
BASEIMAGE arg, which can be useful when building multi-arch images.
BASEIMAGE and BASEIMAGE_CORE args, which can be useful when building images for multiple
Windows versions.
The image building process will be triggered when changes to the files in the docker changes
are made (for example, you bump the image version, so a new image is built).
[1] https://github.com/kubernetes/test-infra/blob/master/config/jobs/image-pushing/README.md
Adds support for multiple Windows versions.
The image will be created with the name
gcr.io/k8s-staging-csi-secrets-store/driver:VERSION
.Bumps the driver's image version.
Related PR: kubernetes/test-infra#17335