[TEST] skipping GA checks with labels #1498
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1498 +/- ##
=======================================
Coverage 97.06% 97.06%
=======================================
Files 82 82
Lines 7739 7739
=======================================
Hits 7512 7512
Misses 227 227 Continue to review full report at Codecov.
|
|
@namurphy @StanczakDominik I'm curious what both of your takes are on this setup. I have my own takes on it, but I want to hear yours before I give my thoughts. |
|
Oh, and feel free to play with the labels to see how they work. |
|
I was briefly confused about what checks are running for General Atomics... I'll give this a look through today. |
There was a problem hiding this comment.
The overall idea looks great! A couple questions for the implementation, and I think we could also give the labels descriptions in https://github.com/PlasmaPy/PlasmaPy/labels.
| SKIP_DOCUMENTATION: ${{ env.SKIP_DOCUMENTATION }} | ||
| SKIP_COMPREHENSIVE: ${{ env.SKIP_COMPREHENSIVE }} | ||
| SKIPS_ALLOWED: ${{ env.SKIPS_ALLOWED }} | ||
| SKIPS_DONE: ${{ env.SKIPS_DONE }} |
There was a problem hiding this comment.
so if I understand it correctly, this skips-allowed job does the logic, puts the result into env vars, and this outputs field, which I'm seeing for the first time, lets you pass those env vars to the next stage. That's reasonable.
There was a problem hiding this comment.
Yep. I originally tries this by defining env on the workflow level, but it turns out the workflow level env just acts as an initialization for the job level env. So, if I modify an environment variable in job1 then that doesn't carry over to job2. The only way I've found that I can pass this information between jobs is using outputs for the job.
| run: | | ||
| echo "Is skipping tests allowed: ${{ env.SKIPS_ALLOWED }}" | ||
| if [[ "$SKIPS_ALLOWED" == "false" && "$SKIPS_DONE" == "true" ]]; then | ||
| echo "Some tests are skipped, but skipping needs to be removed before merging!!" |
There was a problem hiding this comment.
I'd love to make this message more actionable by doing something like this:
| echo "Some tests are skipped, but skipping needs to be removed before merging!!" | |
| echo "Some tests are skipped - either unskip them before merging or add $SKIPS_ALLOWED_LABEL if you're willing to live with the consequences!!" |
but I expect it won't work because SKIPS_ALLOWED_LABEL is "scoped" locally in another job... right?
There was a problem hiding this comment.
I believe this would work. I originally didn't write something like this because I only wanted us to know about the "skips allows" functionality. Only we should be adding that label and not the common contributor.
| if: ${{ needs.skips-allowed.outputs.SKIP_COMPREHENSIVE == 'false' }} | ||
| needs: [initial-tests, skips-allowed] |
There was a problem hiding this comment.
So if SKIP_COMPREHENSIVE is false, this needs both of these... oh, no, right, this runs conditionally only if that var is false, correct?
| @@ -101,6 +176,8 @@ jobs: | |||
| with: | |||
| file: ./coverage.xml | |||
| documentation: | |||
| if: ${{ needs.skips-allowed.outputs.SKIP_DOCUMENTATION == 'false' }} | |||
| needs: skips-allowed | |||
There was a problem hiding this comment.
What I don't understand is... does this only run if you're enabling skips-allowed via GA Skip Merge Allowed? Suppose I don't skip anything, does this still run?
There was a problem hiding this comment.
The skips-allowed job always runs regardless of labeling, and will never fail. I breakout that "merge-able failure" into a separate job since if it fails any dependent jobs would automatically skip.
Since the outputs are defined using the job's env, the outputs are automatically initialized with the env values.
|
This pull request will be closed in 14 days due to a year of inactivity unless the stale label or comment is removed. |
github-actions
bot
added
the
Stale
github-actions
bot
removed
the
Stale
This PR puts a framework into the CI Test workflow that allows parts of the tests to be skipped based on labels added to the PR.
The motivation for this prototype came about when the GA Documentation Test was failing due to a recent release of
jinja2and not related to what the contributor was doing. In this scenario we were completely block from merging the PR until the test was resolved, luckily in that case it was an easy fix.Another upside to this approach, we can turn off comprehensive tests until we reach the end of a PR development. This should allows to both expand PR tests while not overloading our resources.
I did build a job into the workflow that does fail if other jobs are skipped, unless the "GA Skip Merge Allowed" label is added to the PR. This should add some redundancy against us unintentionally merging a PR with skipped tests.