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

i#4549 GA: Configure failure notifs to dynamorio-devs. #4631

Merged
merged 11 commits into from
Dec 20, 2020

Conversation

abhinav92003
Copy link
Contributor

Uses the dawidd6/action-send-mail@v2 action to send notifications via email to dynamorio-devs for all failures in CI or package workflows.

This is the final piece in the migration to Github Actions.

Fixes: #4549

Uses the dawidd6/action-send-mail@v2 action to send notifications via email to dynamorio-devs for all failures in CI or package workflows.

This is the final piece in the migration to Github Actions.

Fixes: #4549
@abhinav92003
Copy link
Contributor Author

@derekbruening Will sync offline to add the two new Github secrets (for username and password of the email account) to the DynamoRIO repo settings.

One caveat of this approach, for PRs from forked repos:

Secrets are not passed to workflows that are triggered by a pull request from a fork.

I haven't tested behaviour on forked repo PRs yet, but it probably means that we won't get these notification emails for CI failures on those.
But the test suites still run as expected. Note that the Github secret is currently not available in the DynamoRIO repo; but in the above commits, the failed workflows do have complete logs from failed tests, and the successful workflows aren't affected.

@abhinav92003 abhinav92003 marked this pull request as ready for review December 17, 2020 08:35
@abhinav92003 abhinav92003 changed the title i4549 GA: Configure failure notifs to dynamorio-devs. i#4549 GA: Configure failure notifs to dynamorio-devs. Dec 17, 2020
@derekbruening
Copy link
Contributor

I haven't tested behaviour on forked repo PRs yet, but it probably means that we won't get these notification emails for CI failures on those.

We want the list emails for pushes to master (not important for PR failures), and I would assume that a forked repo will always be merged by a project member into master and that merge will trigger the email?

.github/workflows/ci-aarchxx.yml Outdated Show resolved Hide resolved
.github/workflows/ci-aarchxx.yml Outdated Show resolved Hide resolved
core/arch/aarchxx/emit_utils.c Outdated Show resolved Hide resolved
@abhinav92003
Copy link
Contributor Author

We want the list emails for pushes to master (not important for PR failures), and I would assume that a forked repo will always be merged by a project member into master and that merge will trigger the email?

The master merge done by a project member should trigger the email on failure, I believe.

I'm going to try to test our assumptions about forked repo PRs but creating such a PR with i4549-failure-email-notifs as target.

Thanks for adding the GIthub Secrets. Will update the PR when testing is complete and ready for review.

@abhinav92003
Copy link
Contributor Author

Observations from testing for forked repo PRs; see PR #4634 which was forked from abhinav92003/dynamorio and merged into DynamoRIO/dynamorio:i4549-failure-email-notifs for testing:

  • Secrets from DynamoRIO/dynamorio are not available to any forked repo PRs. This means that forked repo PRs won't be able to send failure notif emails (see CI logs on PR i#4549: fork repo PR notif testing #4634); unless the forked repo also defines the same Secret variables as DynamoRIO/dynamorio, in which case that other email address will be used. (See other comment thread: This is probably okay as we don't want to send failure notif emails on non-master branches anyway.) We should rename our Secret vars to have a DYNAMORIO_ prefix.

  • If CI succeeds, the email notif issue doesn't hinder green-ness of the checks.

  • When the PR is merged to a DR branch, it does send failure notif emails (see commit b9344bc above).

It is better to have a DYNAMORIO_ prefix on these variables to reduce chances of conflict with a forked repo's Secret vars.
@abhinav92003
Copy link
Contributor Author

abhinav92003 commented Dec 19, 2020

Some other observations:

  • Commit SHA for the synchronize event (which we use for triggering CI on PR branch pushes) is different from the hash of the commit that triggered the synchronize event.
    E.g., following is an email to dynamorio-devs. Note that the SHA doesn't match any commit on PR i#4549 GA: Configure failure notifs to dynamorio-devs. #4631.
Github Actions CI workflow run FAILED!
Workflow: ci-aarchxx/a64-on-x86
Repository: DynamoRIO/dynamorio
Branch ref: refs/pull/4631/merge
SHA: dbb985c6121274ae0e30bda2fc80c0afea062d18
Triggering actor: abhinav92003
Triggering event: pull_request
Run Id: 431475676
See more details on github.com/DynamoRIO/dynamorio/actions/runs/431475676
  • clang-format doesn't seem to fail after the PR is pushed to master, even though it did fail on the PR. Found this while testing on my forked repo; note the failure on https://github.com/abhinav92003/dynamorio/pull/1, but not on the corresponding merge commit. @derekbruening this is probably because clang-format looks only at the changed lines wrt base branch?

  • The automatic emails sent by Github Actions to individuals (the ones titled "[DynamoRIO/dynamorio] PR run failed: ci-xxx") has [email protected] as its recipient. But I don't see such emails for any PR other than my own (@derekbruening can you please confirm?) . So, there will be duplication of failure notif email for the PR author if they are subscribed to dynamorio-devs too.

@derekbruening
Copy link
Contributor

  • clang-format doesn't seem to fail after the PR is pushed to master, even though it did fail on the PR. Found this while testing on my forked repo; note the failure on abhinav92003#1, but not on the corresponding merge commit. @derekbruening this is probably because clang-format looks only at the changed lines wrt base branch?

Sounds right -- yes, it does a "git diff -U0 origin/master".

  • The automatic emails sent by Github Actions to individuals (the ones titled "[DynamoRIO/dynamorio] PR run failed: ci-xxx") has [email protected] as its recipient. But I don't see such emails for any PR other than my own (@derekbruening can you please confirm?) . So, there will be duplication of failure notif email for the PR author if they are subscribed to dynamorio-devs too.

My "PR run failed" and "Run failed" (manually triggered) emails are from: Derek Bruening <[email protected]>, to the same recipient you cite, and cc-ed to [email protected]. Like you I have not received any from other people's PR's. Duplication seems fine: we're talking about merges to master, not PR failures, so they should be relatively rare.

@abhinav92003
Copy link
Contributor Author

abhinav92003 commented Dec 19, 2020

Verified using a simple cron config1 that it also has github.ref == 'refs/heads/master', for both the scheduled trigger and the manual trigger.

@abhinav92003
Copy link
Contributor Author

run arm tests

@abhinav92003 abhinav92003 merged commit db19e2b into master Dec 20, 2020
@abhinav92003 abhinav92003 deleted the i4549-failure-email-notifs branch December 20, 2020 02:03
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.

Migrate ASAP from Travis CI as it no longer offers free OSS CI and will stop working very soon
2 participants