-
Notifications
You must be signed in to change notification settings - Fork 50
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
Run tests as separate jobs #813
Run tests as separate jobs #813
Conversation
✅ Deploy Preview for conda-store canceled.
|
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.
Just a few comments, otherwise this looks like it will really help the CI be more useful. Thanks 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.
We should perhaps rename it to build_docker.yaml
as it is not testing the Docker image per se
@@ -0,0 +1,62 @@ | |||
name: "Test build Docker 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.
name: "Test build Docker image" | |
name: "Build and push Docker 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.
Had a similar idea as well, but decided against it to avoid confusion. We already have a job called build_and_push_docker_image
that's part of release.yaml
. This job was part of tests.yaml
, so I prefixed it with "Test" to see which is which. AFAICT, it also doesn't push anything because of push: false
, so looks like the intention here is to just test whether a docker container can be built.
In the interest of time, I'm going to merge this as is since it's already approved, but this can always be resolved in followup PRs.
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.
But it is not testing the docker image just building it the fact that we have a step/job named build and push is inconsequential to having a workflow named build or build and push.
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.
Yeah, I think a better name might be "Build Docker image", but we already have a file called build_docker_image.yaml
with the name "Publish Dev Docker Images".
I think my preferred solution would be making all these consistent, so that the name matches the filename and there are no clashes between files. Something like this (untested):
- "Publish dev Docker image" -> publish_dev_docker_image.yaml (renamed from "Publish Dev Docker Images")
- "Build Docker image" -> build_docker_image.yaml (the workflow discussed in this thread)
@peytondmurray Hey Peyton, I'm currently on PTO, would you be comfortable taking over this in a separate PR? The task would be coming up with a consistent scheme for names and filenames of docker workflows.
This reverts commit 2f217fe.
18245eb
to
d36ff4c
Compare
Description
This pull request:
Pull request checklist
Additional information
Also see #803.
How to test