-
Notifications
You must be signed in to change notification settings - Fork 102
Create a separate "Flutter Presubmits" checkrun for the unified checkrun flow. #5022
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
Changes from all commits
4c62da5
6929163
0b257d8
8734e67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,10 @@ class Scheduler { | |
| 'If you need to merge your PR without tests (a rare situation, typically ' | ||
| 'an emergency), then you can use the `emergency` label.'; | ||
|
|
||
| /// Briefly describes what the "Flutter Presubmits" check is for. | ||
| static const String kFlutterPresubmitsDescription = | ||
| 'Flutter Presubmits unified check run.'; | ||
|
|
||
| /// Ensure [commits] exist in Cocoon. | ||
| /// | ||
| /// If the commit already exists, it is ignored. | ||
|
|
@@ -332,6 +336,10 @@ class Scheduler { | |
|
|
||
| final slug = pullRequest.base!.repo!.slug(); | ||
|
|
||
| final isUnifiedCheckRun = _config.flags.isUnifiedCheckRunFlowEnabledForUser( | ||
| pullRequest.user!.login!, | ||
| ); | ||
|
|
||
| // The MQ only waits for "required status checks" before deciding whether to | ||
| // merge the PR into the target branch. This required check added to both | ||
| // the PR and to the merge group, and so it must be completed in both cases. | ||
|
|
@@ -340,12 +348,10 @@ class Scheduler { | |
| pullRequest.head!.sha!, | ||
| // Override details url of merge queue guard check for users with unified | ||
| // check run flow enabled | ||
| detailsUrl: | ||
| _config.flags.isUnifiedCheckRunFlowEnabledForUser( | ||
| pullRequest.user!.login!, | ||
| ) | ||
| detailsUrl: isUnifiedCheckRun | ||
| ? 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${slug.name}&sha=${pullRequest.head!.sha}' | ||
| : null, | ||
| isUnifiedCheckRun: isUnifiedCheckRun, | ||
| ); | ||
|
|
||
| // Track if we should unlock the merge group lock in case of non-fusion or | ||
|
|
@@ -611,7 +617,11 @@ class Scheduler { | |
| '${contentHash != null ? ', contentHash: $contentHash' : ''})'; | ||
| log.info('$logCrumb: scheduling merge group checks'); | ||
|
|
||
| final lock = await lockMergeGroupChecks(slug, headSha); | ||
| final lock = await lockMergeGroupChecks( | ||
| slug, | ||
| headSha, | ||
| isUnifiedCheckRun: false, | ||
| ); | ||
|
|
||
| // If the repo is not fusion, it doesn't run anything in the MQ, so just | ||
| // close the merge group guard. | ||
|
|
@@ -827,8 +837,9 @@ $s | |
| RepositorySlug slug, | ||
| String headSha, { | ||
| String? detailsUrl, | ||
| required bool isUnifiedCheckRun, | ||
| }) async { | ||
| return _githubChecksService.githubChecksUtil.createCheckRun( | ||
| final mqGuard = await _githubChecksService.githubChecksUtil.createCheckRun( | ||
| _config, | ||
| slug, | ||
| headSha, | ||
|
|
@@ -837,8 +848,43 @@ $s | |
| title: Config.kMergeQueueLockName, | ||
| summary: kMergeQueueLockDescription, | ||
| ), | ||
| detailsUrl: detailsUrl, | ||
| detailsUrl: isUnifiedCheckRun ? null : detailsUrl, | ||
| ); | ||
|
|
||
| final flutterPresubmits = await _githubChecksService.githubChecksUtil | ||
| .createCheckRun( | ||
| _config, | ||
| slug, | ||
| headSha, | ||
| Config.kFlutterPresubmitsName, | ||
| output: const CheckRunOutput( | ||
| title: Config.kFlutterPresubmitsName, | ||
| summary: kFlutterPresubmitsDescription, | ||
| ), | ||
| detailsUrl: isUnifiedCheckRun ? detailsUrl : null, | ||
| ); | ||
|
|
||
| if (isUnifiedCheckRun) { | ||
| // Skip MQ Guard | ||
| await _githubChecksService.githubChecksUtil.updateCheckRun( | ||
| _config, | ||
| slug, | ||
| mqGuard, | ||
| status: CheckRunStatus.completed, | ||
| conclusion: CheckRunConclusion.success, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer using neutral or skipped?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how the required checks are configured. Will they still allow the PR to merge if the status is neutral or skipped?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the answer is "sometimes": https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks Whether the check blocks when it is skipped depends on where it comes from, but it actually doesn't say anything about when its status is set programatically through the GH API. I think I'm just going to leave these as is, so that we don't break anything for anyone. |
||
| ); | ||
| return flutterPresubmits; | ||
| } else { | ||
| // Skip Flutter Presubmits | ||
| await _githubChecksService.githubChecksUtil.updateCheckRun( | ||
| _config, | ||
| slug, | ||
| flutterPresubmits, | ||
| status: CheckRunStatus.completed, | ||
| conclusion: CheckRunConclusion.success, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto neutral/skipped |
||
| ); | ||
| return mqGuard; | ||
| } | ||
| } | ||
|
|
||
| /// Creates a pending check run for "Awaiting CICD label" if it doesn't exist. | ||
|
|
@@ -1592,11 +1638,12 @@ $stacktrace | |
| ); | ||
| final name = checkRunEvent.checkRun!.name; | ||
| var success = false; | ||
| if (name == Config.kMergeQueueLockName) { | ||
| if (name == Config.kMergeQueueLockName || | ||
| name == Config.kFlutterPresubmitsName) { | ||
| final slug = checkRunEvent.repository!.slug(); | ||
| final checkSuiteId = checkRunEvent.checkRun!.checkSuite!.id!; | ||
| log.debug( | ||
| '$logCrumb: Requested re-run of "${Config.kMergeQueueLockName}" for ' | ||
| '$logCrumb: Requested re-run of "$name" for ' | ||
| '$slug / $checkSuiteId - ignoring', | ||
| ); | ||
| success = true; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to create both? Is the idea that we'll have "MQG" and "PSG"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we need to create both is that both will be required checks for the master branch.