-
Notifications
You must be signed in to change notification settings - Fork 79
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
GH docker based reusable CI workflows. #993
GH docker based reusable CI workflows. #993
Conversation
f925ea1
to
3f41010
Compare
3f41010
to
b5fb70b
Compare
…e implementation is ready to be merged.
…re until the implementation is ready to be merged.
…ions of docker which do not support this idiom.
…w parameterised on USE_CUDA. 3. +.dockerignore aliased to .gitignore.
…sha arg from build-docker-image.
36196dc
to
bf0aae9
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.
We should be bumping the versions dependency in its own PR.
@@ -5,7 +5,7 @@ | |||
"git_url" : "https://github.com/nv-legate/legate.core.git", | |||
"git_shallow": false, | |||
"always_download": false, | |||
"git_tag" : "a405f595603238c8557cb5fefd3981d190a2fb1d" | |||
"git_tag" : "4b79075eb5d7035d501c334c87a87939af79abc2" |
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.
Is there something in legate that needs to be updated for this?
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.
It looks like we do need a later version of legate.core than what is currently specified in versions.json.: https://github.com/nv-legate/cunumeric/pull/993/checks#step:7:50. To be more specific we need the file legate/continuous_integration/build-docker-image
.
The selected SHA does not directly point to the specific change which introduced the aforementioned dependency but it advances legate.core to the point where we know we have a successful build using build-docker-image. Hope that makes sense.
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.
Yup, that makes sense. Please proceed.
…ded for upcoming change: nv-legate#993.
9037842
to
bf0aae9
Compare
|
||
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
BASE_IMAGE: rapidsai/devcontainers:23.06-cpp-cuda11.8-mambaforge-ubuntu22.04 |
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.
It looks like 23.10 is the current version of devcontainer. We can update it here or in a separate PR.
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 will update it in a separate PR.
-H "Accept: application/vnd.github+json" \ | ||
-H "Authorization: Bearer ${{ github.token }}"\ | ||
-H "X-GitHub-Api-Version: 2022-11-28" \ | ||
https://api.github.com/orgs/${{ github.repository_owner }}/packages/container/$PACKAGE_NAME/versions/$PACKAGE_VERSION_ID |
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 am not 100% sure I understand what is going on. So in the build step, we build the image and push it. Is this the same image we delete in this step?
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.
Is this the same image we delete in this step?
Yes. We don't need the image if all the tests succeed. After I have submitted #1022 if some test does not succeed you will be able to download the image and reproduce the problem locally. In addition to this I will create a separate CI job to delete any unused images after a certain period of time.
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 see. How is this job prevented from running if the tests fail? I see that the cleanup call says always
. I may be missing something.
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 have removed always
in #1022.
No description provided.