-
Notifications
You must be signed in to change notification settings - Fork 139
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 github action to push docker images with latest tag. #370
Conversation
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
6fa57c5
to
4a5dc5e
Compare
122a094
to
9e69e11
Compare
cat << EOF | ||
"Incorrect number of parameters provided. The required parameters are versions_file and pipeline_path. | ||
$1 $2 |
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.
should we add more explanation here? instead just $1 and $2
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.
Thanks I removed the redundant printing...
FROM docker.io/rayproject/ray:2.24.0-py310 | ||
ARG BASE_IMAGE=docker.io/rayproject/ray:2.24.0-py310 | ||
|
||
FROM ${BASE_IMAGE} |
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.
wow, that's very suitable.
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
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.
@revit13 and I spoke and agreed we will try and simplify things by always publishing the "latest" docker images if a) it is a merge into dev and b) the make test-image passes.
Publishing will be done after make test-image
only for dev merges and the tag in this case will be latest
. The "releases" will be handled manually (for now?) and publishing will be done out of releases/vX.Y.Z
branch. But perhaps this can somehow be automated as well? Maybe not using latest
tag?
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
a0601c0
to
721b043
Compare
Signed-off-by: Revital Sur <eres@il.ibm.com>
@daw3rd I changed the action to push as described above. Please let me know if thats OK. I also added a condition to push the images upon creating a new tag which should cover publishing for release. Thanks |
Signed-off-by: Revital Sur <eres@il.ibm.com>
.PHONY: .defaults.docker-save-image | ||
.defaults.docker-save-image: | ||
@# Help: Save docker image as tar file to ${ARTIFACTS_DIR} directory. | ||
mkdir -p ${ARTIFACTS_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.
Now that we're trying to add checks, can you add a check for DOCKER_IMAGE_NAME. Maybe this (and others) goes at the top of this file though instead of at each referencing target? I guess its the "global" ones like this that could go at the top of this file?
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.
Checking DOCKER_IMAGE_NAME
in the top of this file results in an error as its is set in the Makefiles that generate Docker images and include .make.defauls, for example in .make.transforms
. So I added it to image
and publish
targets.
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
Why are these changes needed?
This PR automates the process of pushing Docker images with
latest
tag upon each merge to the development branch. It accomplishes this by storing the images generated during the execution of thetest-image
target using thehttps://github.com/actions/upload-artifact
action and subsequently downloading and pushing them to the repository following the completion of testing.It also changes ray docker images to have a base image and thus avoids the need to call update_dockerfile.sh script to update the ray version in set-versions.
the pushed images rae:
Related issue number (if any).
#176