-
Notifications
You must be signed in to change notification settings - Fork 20
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
Report CI failures through GitHub issues #413
base: master
Are you sure you want to change the base?
Conversation
Report failures for all jobs but `write-install-commands`, which is more of an intermediate helper than something to report errors for. If this gets noisy for any particular job, it can be ignored by removing the job id from the dependency list.
.github/workflows/ci.yaml
Outdated
- test-standalone | ||
- doc | ||
- release | ||
uses: nextstrain/.github/.github/workflows/report-failure.yaml |
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 believe the @ref
is required by uses:
.
uses: nextstrain/.github/.github/workflows/report-failure.yaml | |
uses: nextstrain/.github/.github/workflows/report-failure.yaml@master |
failure: ${{ contains(needs.*.result, 'failure') }} | ||
|
||
# The condition evaluates to true when all results are 'success', | ||
# 'cancelled', or 'skipped'¹. This is not ideal, since a workflow that is | ||
# cancelled before there are any successes will still signal a success to | ||
# this job, however the chance of that happening should be very low given | ||
# that this is job is conditioned on the default branch. | ||
# ¹ <https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#jobs-context> | ||
success: ${{ !contains(needs.*.result, 'failure') }} |
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.
Especially in consideration of nextstrain/.github#121 (comment), I'd think to define this more like:
with:
# At least one success, no failures.
ok: ${{ contains(needs.*.result, 'success') && !contains(needs.*.result, 'failure') }}
Or maybe stepping back a bit and doing this sort of thing instead to not "redefine" what success/failure means:
report-failure:
if: ${{ failure() && github.ref_name == github.event.repository.default_branch }}
needs:
- lint
- test-source
- build-dist
- build-standalone
- test-dist
- test-standalone
- doc
- release
uses: nextstrain/.github/.github/workflows/report-failure.yaml@master
with:
ok: false
title: CI failure
report-success:
if: ${{ success() && github.ref_name == github.event.repository.default_branch }}
needs:
- lint
- test-source
- build-dist
- build-standalone
- test-dist
- test-standalone
- doc
- release
uses: nextstrain/.github/.github/workflows/report-failure.yaml@master
with:
ok: true
title: CI failure
I wonder if the above can be reduced in verbosity, but I just wanted to show the idea of reusing the failure()
and success()
functions. The docs imply they can only be used in if:
fields.
needs: | ||
- lint | ||
- test-source | ||
- build-dist | ||
- build-standalone | ||
- test-dist | ||
- test-standalone | ||
- doc | ||
- release |
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.
non-blocking
It feels like it's going to be easy to forget to update this if we add new jobs. :/
Description of proposed changes
Report failures for all jobs but
write-install-commands
, which is more of an intermediate helper than something to report errors for.If this gets noisy for any particular job, it can be ignored by removing the job id from the dependency list.
Related issue(s)
Closes #412
Checklist