[app_dart] Schedule prod tasks from GitHub webhooks#1109
[app_dart] Schedule prod tasks from GitHub webhooks#1109CaseyHillers merged 5 commits intoflutter:masterfrom
Conversation
CaseyHillers
changed the title
| const GithubWebhook(Config config, this.buildBucketClient, this.luciBuildService, this.githubChecksService) | ||
| GithubWebhook(Config config, | ||
| {@required this.buildBucketClient, | ||
| @visibleForTesting Scheduler schedulerValue, |
There was a problem hiding this comment.
TIL you can apply @visibleForTesting to a param
Yes. This means commits can either be scheduled via the cron or or the GitHub webhook for when PRs are closed. After I validate the webhook approach, I'll remove the cron method. |
| ); | ||
| } else { | ||
| // Merged pull requests can be added to CI. | ||
| await scheduler.addPullRequest(pr); |
There was a problem hiding this comment.
Should we be listening for merged PRs, or commits to ____ branch?
There was a problem hiding this comment.
I don't think there's a commit webhook event - https://docs.github.com/en/developers/webhooks-and-events/webhook-events-and-payloads
The PR event provides a lot of data, and it provides the release branch info. I'll add a test to verify release PRs.
There was a problem hiding this comment.
Offline conversation. There is a commit webhook called push - https://docs.github.com/en/developers/webhooks-and-events/webhook-events-and-payloads#push
It looks like the GitHub library doesn't support that event yet, so I'm going to look into adding it
There was a problem hiding this comment.
I looked into the push events on the archive table, and they're not the solution. We need to support this event + push events to get 100% coverage. From the last 90 days, push events made up 0% of commits that need to be tested by infra.
I'm going to follow up and modify the cron job from every minute to every hour, and have it check for any missing commits. Right now it only adds newer, missing commits.
There was a problem hiding this comment.
Filed flutter/flutter#78351 for tracking the PushEvent addition
There was a problem hiding this comment.
We sync'ed up offline again, and triggering based on PR merged even SGTM
| timestamp: pr.mergedAt.millisecondsSinceEpoch, | ||
| ); | ||
|
|
||
| if (await _commitExistsInDatastore(mergedCommit)) { |
There was a problem hiding this comment.
Yes. Since commits can be scheduled via (1) cron job and (2) webhook triggers we should ensure we haven't already added the commit. GitHub will retry commits if cocoon sends a non-2** response, so we should handle this case gracefully.
There was a problem hiding this comment.
That makes sense. I guess we can still guarantee the order of commits with these two scheduling logics, right?
christopherfujino
left a comment
christopherfujino
left a comment
There was a problem hiding this comment.
nit about expanding dartdoc for _commitExistsInDatastore, otherwise LGTM
CaseyHillers
added
the
waiting for tree to go green
3 participants
Given [PullRequest], add it to Cocoon so tasks can be scheduled against it.
Issues
flutter/flutter#76140 - Move from cron method to webhooks
flutter/flutter#77385 - Webhook method will no longer have this issue. The GitHub refresh cron doesn't dynamically add commits