Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class PresubmitCompletedJob {
cocoon_checks.CheckRun get checkRun {
return cocoon_checks.CheckRun(
id: checkRunId,
name: isUnifiedCheckRun ? Config.kMergeQueueLockName : name,
name: isUnifiedCheckRun ? Config.kFlutterPresubmitsName : name,
headSha: sha,
conclusion: status.toConclusion(),
checkSuite: CheckSuite(
Expand Down
4 changes: 4 additions & 0 deletions app_dart/lib/src/service/config.dart
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ interface class Config extends DynamicallyUpdatedConfig {
/// workflow.
static const String kMergeQueueLockName = 'Merge Queue Guard';

/// A required check that stays in pending state until all presubmit checks pass
/// for users opted into the unified checkrun flow.
static const String kFlutterPresubmitsName = 'Flutter Presubmits';

final CacheService _cache;
final SecretManager _secrets;
final http.Client _httpClient;
Expand Down
65 changes: 56 additions & 9 deletions app_dart/lib/src/service/scheduler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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,
);
Comment on lines 842 to +865
Copy link
Copy Markdown
Member

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"?

Copy link
Copy Markdown
Contributor Author

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.


if (isUnifiedCheckRun) {
// Skip MQ Guard
await _githubChecksService.githubChecksUtil.updateCheckRun(
_config,
slug,
mqGuard,
status: CheckRunStatus.completed,
conclusion: CheckRunConclusion.success,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Prefer using neutral or skipped?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ void main() {
expect(check.checkSuiteId, 456);
expect(check.headBranch, 'gh-readonly-queue/master/pr-123-abc');
expect(check.isUnifiedCheckRun, true);
expect(check.checkRun.name, Config.kMergeQueueLockName);
expect(check.checkRun.name, Config.kFlutterPresubmitsName);
expect(check.buildNumber, 0);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3351,7 +3351,7 @@ void foo() {
status: CheckRunStatus.completed,
conclusion: CheckRunConclusion.success,
),
).called(1);
).called(2);

expect(
log,
Expand Down
Loading
Loading