Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

victorlin
Copy link
Member

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

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.
- test-standalone
- doc
- release
uses: nextstrain/.github/.github/workflows/report-failure.yaml
Copy link
Member

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:.

Suggested change
uses: nextstrain/.github/.github/workflows/report-failure.yaml
uses: nextstrain/.github/.github/workflows/report-failure.yaml@master

Comment on lines +406 to +414
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') }}
Copy link
Member

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.

Comment on lines +395 to +403
needs:
- lint
- test-source
- build-dist
- build-standalone
- test-dist
- test-standalone
- doc
- release
Copy link
Member

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. :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open an issue on failure of scheduled CI check
2 participants