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

Reusable notification workflow #665

Merged
merged 14 commits into from
Jun 16, 2024
Merged

Conversation

samwinebrake
Copy link
Collaborator

@samwinebrake samwinebrake commented Mar 19, 2024

This PR has a couple new features:

  • new reusable workflow .github/workflows/user_notification_system.yml with two capabilities
    • extract user emails from both web submissions and non-web submissions
    • failure notification system
  • failure check and email function in brainscore_vision/submission/actions_helpers.py
  • trigger of failure notification system whenever any check fails in .github/workflows/check_if_pr_is_automergeable.yml
  • use of new email extraction workflow in .github/workflows/score_new_plugins.yml that passes correct email on to Jenkins

Closes issue #80 in core.

NOTE:

  • In the future, these workflows will be moved to core in order to allow for centralized github actions.
  • The past Travis notification system will be removed by hard release.

@samwinebrake samwinebrake changed the title Sw/reusable notification workflow Reusable notification workflow Mar 19, 2024
@samwinebrake samwinebrake self-assigned this Mar 19, 2024
@samwinebrake samwinebrake requested a review from mschrimpf March 20, 2024 13:29
@mschrimpf
Copy link
Member

@kvfairchild knows github actions much better than me, will defer to her review

Copy link
Contributor

@kvfairchild kvfairchild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, this has the potential to send a LOT of duplicate failure notification emails; for instance, if the first test suite fails, the user will receive the exact same failure email for every subsequent status update, regardless of whether or not those test suites pass. Suggest only sending email for first detected failure.

.github/workflows/check_if_pr_is_automergeable.yml Outdated Show resolved Hide resolved
@samwinebrake
Copy link
Collaborator Author

Screen Shot 2024-05-14 at 11 44 14 AM

Example of failure-notified label shown here (simulating an automerge-web submission that fails tests). When tests fail, the user is emailed and a failure-notified label is added to the PR. I then removed the failure-notified label and re-added it to ensure that testing is skipped when a failure-notified label is added (so that we don't overrun this workflow when an initial failure-notified is added). Lastly, I pushed a new change to the PR showing that the github-actions bot will remove the failure-notified label with new changes. It then emails the user about this new failure wrt the new PR, and readds the failure-notified label. When no changes have occurred to an already failing PR but a new check has failed, the notification workflow will be skipped due to the presence of the failure-notified label.

@kvfairchild does this address your concerns?

Copy link
Contributor

@kvfairchild kvfairchild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this has been thoroughly tested, LGTM!

# - completion of CI checks (status events), OR
# - tagging with "automerge" or "automerge-web" labels
# - tagging with "automerge" or "automerge-web" labels, OR
# - updates to current PRs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't changes to open PRs automatically result in CI checks?

# - completion of CI checks (status events), OR
# - tagging with "automerge" or "automerge-web" labels
# - tagging with "automerge" or "automerge-web" labels, OR
# - updates to current PRs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# - updates to current PRs
# - updates to current PRs ("synchronize" events)

@samwinebrake samwinebrake merged commit 031a447 into master Jun 16, 2024
10 checks passed
@samwinebrake samwinebrake deleted the sw/reusable_notification_workflow branch June 16, 2024 19:02
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.

3 participants