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.

Sign up for GitHub

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

Jump to bottom

[ci] Filter GitHub pull request events #13552

Merged
merged 1 commit into from
Oct 16, 2018
Merged

[ci] Filter GitHub pull request events #13552

merged 1 commit into from
Oct 16, 2018

Conversation

jugglinmike
Copy link
Contributor

We've been validating pull requests on Taskcluster far more often than we need to. This dummy pull request submitted to Bocoup's fork of WPT demonstrates the various events. The GitHub UI only displays the latest status, so it's necessary to query the HTTP API to see the complete status history:

$ curl --silent https://api.github.com/repos/bocoup/wpt/commits/e9dcdf611/statuses | \
  python -c 'import sys, json; print "\n".join(["{created_at} {description}".format(**x) for x in json.loads(sys.stdin.read())])'
output 
2018-10-16T18:22:02Z TaskGroup: success
2018-10-16T18:14:49Z The Travis CI build passed
2018-10-16T18:14:09Z The Travis CI build passed
2018-10-16T18:09:20Z The Travis CI build is in progress
2018-10-16T18:08:51Z TaskGroup: success
2018-10-16T18:08:51Z The Travis CI build is in progress
2018-10-16T18:08:50Z The Travis CI build is in progress
2018-10-16T18:08:50Z The Travis CI build is in progress
2018-10-16T18:08:49Z The Travis CI build is in progress
2018-10-16T18:08:49Z The Travis CI build is in progress
2018-10-16T18:08:49Z The Travis CI build is in progress
2018-10-16T18:08:48Z The Travis CI build is in progress
2018-10-16T18:08:48Z The Travis CI build is in progress
2018-10-16T18:08:48Z The Travis CI build is in progress
2018-10-16T18:08:47Z The Travis CI build is in progress
2018-10-16T18:08:46Z TaskGroup: Pending (for pull_request.reopened)
2018-10-16T18:07:59Z TaskGroup: success
2018-10-16T18:07:53Z TaskGroup: Pending (for pull_request.closed)
2018-10-16T18:06:55Z TaskGroup: success
2018-10-16T18:06:49Z TaskGroup: Pending (for pull_request.unassigned)
2018-10-16T18:06:37Z TaskGroup: success
2018-10-16T18:06:06Z TaskGroup: Pending (for pull_request.assigned)
2018-10-16T18:05:25Z TaskGroup: success
2018-10-16T18:04:56Z TaskGroup: Pending (for pull_request.review_request_removed)
2018-10-16T18:04:03Z TaskGroup: success
2018-10-16T18:03:56Z TaskGroup: Pending (for pull_request.review_requested)
2018-10-16T18:02:06Z TaskGroup: success
2018-10-16T18:01:55Z TaskGroup: Pending (for pull_request.edited)
2018-10-16T18:01:51Z TaskGroup: Pending (for pull_request.edited)
2018-10-16T18:01:07Z The Travis CI build passed

Validating commits in response to many of these events is inefficient. The response to the "closed" event is particularly concerning. Many people (and all scripts) delete the base branch immediately after merging a pull request. The subsequent validation attempt cannot run in this state, so it always fails. Because the GitHub UI only displays the latest commit status, this gives a false impression of the status at the moment the patch was accepted (for a real-world example of this, see gh-13045).

This dummy pull request demonstrates the behavior after this patch is applied.

$ curl --silent https://api.github.com/repos/bocoup/wpt/commits/03b1b7456a868d50adc/statuses | \
  python -c 'import sys, json; print "\n".join(["{created_at} {description}".format(**x) for x in json.loads(sys.stdin.read())])'
output 
2018-10-16T20:30:36Z The Travis CI build passed
2018-10-16T20:28:01Z The Travis CI build passed
2018-10-16T20:26:25Z The Travis CI build could not complete due to an error
2018-10-16T20:26:05Z TaskGroup: success
2018-10-16T20:23:52Z The Travis CI build is in progress
2018-10-16T20:23:51Z The Travis CI build is in progress
2018-10-16T20:23:49Z The Travis CI build is in progress
2018-10-16T20:23:48Z The Travis CI build is in progress
2018-10-16T20:23:46Z The Travis CI build is in progress
2018-10-16T20:23:45Z The Travis CI build is in progress
2018-10-16T20:23:44Z The Travis CI build is in progress
2018-10-16T20:23:43Z The Travis CI build is in progress
2018-10-16T20:23:41Z The Travis CI build is in progress
2018-10-16T20:23:40Z The Travis CI build is in progress
2018-10-16T20:23:37Z TaskGroup: Pending (for pull_request.reopened)
2018-10-16T20:22:55Z The Travis CI build is in progress
2018-10-16T20:21:42Z TaskGroup: success
2018-10-16T20:20:47Z The Travis CI build is in progress
2018-10-16T20:20:46Z The Travis CI build is in progress
2018-10-16T20:20:44Z The Travis CI build is in progress
2018-10-16T20:20:43Z The Travis CI build is in progress
2018-10-16T20:20:42Z The Travis CI build is in progress
2018-10-16T20:20:41Z The Travis CI build is in progress
2018-10-16T20:20:39Z The Travis CI build is in progress
2018-10-16T20:20:36Z The Travis CI build is in progress
2018-10-16T20:20:35Z The Travis CI build is in progress
2018-10-16T20:20:33Z The Travis CI build is in progress
2018-10-16T20:20:29Z TaskGroup: Pending (for pull_request.opened)

The complete list of event "actions" emitted by GitHub (and recognized
by Taskcluster) is [1]:

  • assigned
  • unassigned
  • labeled
  • unlabeled
  • opened
  • edited
  • closed
  • reopened
  • synchronize
  • review_requested
  • review_request_removed

Most of these have no bearing on the code under review, so they should
not trigger validation.

Do not validate commits in response to irrelevant events.

[1] https://docs.taskcluster.net/docs/reference/integrations/taskcluster-github/references/events

@jugglinmike
[ci] Filter GitHub pull request events
52d9fe8 
The complete list of event "actions" emitted by GitHub (and recognized
by Taskcluster) is [1]:

> - assigned
> - unassigned
> - labeled
> - unlabeled
> - opened
> - edited
> - closed
> - reopened
> - synchronize
> - review_requested
> - review_request_removed

Most of these have no bearing on the code under review, so they should
not trigger validation.

Do not validate commits in response to irrelevant events.

[1] https://docs.taskcluster.net/docs/reference/integrations/taskcluster-github/references/events
foolip
foolip approved these changes Oct 16, 2018
View reviewed changes
.taskcluster.yml
echo "Command exited with code $result (failures are allowed while this task is being vetted)."
# Taskcluster responds to a number of events issued by the GitHub API
# which should not trigger re-validation.
$if: event.action in ['opened', 'reopened', 'synchronize']
Copy link
Member

Choose a reason for hiding this comment

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

What is the 'synchronize' event? Just a branch update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find documentation for it, but that's been my experience

Copy link

Choose a reason for hiding this comment

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

That is when there is a push to the branch that the PR is tracking

Copy link
Member

Choose a reason for hiding this comment

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

Alright, thanks @owlishDeveloper!

@jugglinmike, any reason we want to react to any even other than just 'synchronize'? When a PR is first created, is there not both an 'opened' and a 'synchronize' event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in my experience or in the statuses shared above. I chose to include "reopened" to help with a rare case where a pull request is revisited. Re-validating might be desired for nightly releases

Copy link

Choose a reason for hiding this comment

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

I don't think there is a synchronize event when you open a PR, but I cannot confirm that at the moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More anecdotal evidence: none of the 515 WPT pull request commits that have been validated by Taskcluster since 2018-09-13 (the day after we enabled pull request validation) have triggered both "opened" and "synchronize"

@jgraham jgraham merged commit 4f20388 into web-platform-tests:master Oct 16, 2018
@jgraham
Copy link
Contributor

jgraham commented Oct 16, 2018

Ah, this explains something we were seeing with the wpt sync in gecko where we would get a TC notification after the branch merged. Thanks for investigating!

@jugglinmike
Copy link
Contributor Author

Sure thing!

@Hexcles
Copy link
Member

Hexcles commented Oct 17, 2018

LGTM. Thanks, @jugglinmike !

@jugglinmike jugglinmike mentioned this pull request Nov 7, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foolip foolip foolip approved these changes

@owlishDeveloper owlishDeveloper owlishDeveloper approved these changes

@gsnedders gsnedders Awaiting requested review from gsnedders

@jgraham jgraham Awaiting requested review from jgraham

@Hexcles Hexcles Awaiting requested review from Hexcles

Assignees
No one assigned
Labels
infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jugglinmike @jgraham @Hexcles @foolip @owlishDeveloper @wpt-pr-bot