SEAB-5015: Use [skip] to run partial CI build #5833
Conversation
|
If the additional CircleCI tests are required to merge a PR, then the most recent commit cannot contain |
Yes on both counts. I guess you can skip tests on most commits in a PR, but not the last one if you need it reviewed. So not a perfect solution, but much better than before. |
I think the job name is fine, I think "skip" on its own in a commit message is a bit unintuitive. From a Maven perspective, I'd like "skipTests" for example |
I think you'd still want to run the relatively short unit tests job. |
There was a problem hiding this comment.
Conceptually, this looks great to me. Some concerns that I'm not sure how to resolve:
- We'll have to remember to use it; my default muscle memory to, for example, merge a README.md only commit to develop will be to type
[skip ci], and then we'll still have the problem. It would be nice if you could tell CircleCI to ignore[skip ci]on the develop branch, but I don't think there's a way. - We'll have to remember its name; right now I'd have to and look for it in the config.yml file.
- Maybe a more "positive" name? Instead of
skipTests,quickBuild(especially since you are going to be running the unit tests)?
Kinda prefer not making something up, but point taken on unit tests now. |
.circleci/config.yml
Outdated
| @@ -50,43 +50,47 @@ workflows: | |||
| jobs: | |||
| - build: | |||
| <<: *common_filters | |||
| - skip_check: | |||
There was a problem hiding this comment.
I'm okay with the [skipTests] name in the commit message, but I find the CI job name a bit unintuitive when looking at the screenshot since the name is skip_check but it fails if you want to skip the check.
Proposing something like runFullBuild? So it fails if there's [skipTests] in the commit message and passes if there's no skipTests
| echo "export COMMIT_MESSAGE=\"$(git log --format=oneline -n 1 $CIRCLE_SHA1)\"" >> $BASH_ENV | ||
| source $BASH_ENV | ||
| echo "Commit message: $COMMIT_MESSAGE" | ||
| if [[ $COMMIT_MESSAGE =~ ^.*\[skipTests\].*$ ]]; then |
There was a problem hiding this comment.
There's a built in environment variable called CIRCLE_BRANCH that contains the name of the git branch. Could add a check that ignores the skipTests condition if the branch is develop
("run_full_build" seemed more like the act of running the full build, "full_build_check" = check if the full build should be run)
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5833 +/- ##
=============================================
- Coverage 74.35% 70.96% -3.40%
+ Complexity 5253 5019 -234
=============================================
Files 364 364
Lines 19019 19019
Branches 2019 2019
=============================================
- Hits 14142 13496 -646
- Misses 3918 4562 +644
- Partials 959 961 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
There was a problem 9E88 hiding this comment.
Thanks for your work on this! I'm still a little worried that we (or at least I) will continue to use skip ci, and not take advantage of this, but hopefully we'll learn over time. And there's no other option, short of this ticket getting addressed.
There was a problem hiding this comment.
Seems like it might be worth documenting the skipTests feature somewhere so we don't forget about it, but I'm not sure where. Maybe https://github.com/dockstore/dockstore?tab=readme-ov-file#for-dockstore-developers?
Description
When pushing non-code changes, it is ideal to save build minutes by skipping CircleCI tests. However, using the
[skip ci]tag (which skips the entire pipeline) isn't always great because automatic deployment requires a build.One solution is having the option to run a short CI. To achieve this, this PR attempts to implement a custom tag,
[skipTests]: if included in the most recent commit message, CircleCI should skip all jobs excluding the actual build (i.e. skip testing-related jobs). Whether or not to skip testing is decided by an intermediate job calledskip_check: CircleCI should run all tests ifskip_checksucceeds, and should stop after the build ifskip_checkfails (i.e. finds[skipTests]in the commit message).Review Instructions
The most recent commit uses the
[skipTests]tag, so: 1) the build job should succeed, and 2)skip_checkshould fail, meaning the remaining CI jobs should be skipped. (as of PR feedback on Mar 7 2024, theunit_testjob should also succeed)Consider the effectiveness/utility of this functionality and/or how it could be modified/improved for our purposes.
Other thoughts:
[skipTests]is used, something needs to fail in order to skip future jobs, so overall it looks like the pipeline failed. Would this interfere with the build required for deployment? (The original goal was to conditionally run steps in a job, so[skipTests]would just bypass certain actions during the run, but I haven't yet found a way to incorporate the commit message into conditional steps)[skipTests]tag is used, should be run alongside the build/unit_test?skip_check, which may seem a bit unintuitive? (i.e. it fails if it succeeds to find[skipTests], and succeeds if it fails to find[skipTests])Issue
SEAB-5015
Security and Privacy
If there are any concerns that require extra attention from the security team, highlight them here and check the box when complete.
e.g. Does this change...
Please make sure that you've checked the following before submitting your pull request. Thanks!
mvn clean install@RolesAllowedannotation