Skip to content

Commit fe84d6f

Browse files
Reverts "Conditionally lock MQG on flutter/flutter+master only. (#4685)" (#4691)
Reverts: #4685 Initiated by: matanlurey Reason for reverting: Breaks Flutter CI progression from engine -> framework Original PR Author: matanlurey Reviewed By: {jtmcdole} This change reverts the following previous change: Closes flutter/flutter#168867.
1 parent d9c76f2 commit fe84d6f

2 files changed

Lines changed: 37 additions & 29 deletions

File tree

app_dart/lib/src/service/scheduler.dart

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -325,35 +325,27 @@ class Scheduler {
325325
await cancelPreSubmitTargets(pullRequest: pullRequest, reason: reason);
326326

327327
final slug = pullRequest.base!.repo!.slug();
328-
final isFusion = slug == Config.flutterSlug;
329328

330329
// The MQ only waits for "required status checks" before deciding whether to
331330
// merge the PR into the target branch. This required check added to both
332331
// the PR and to the merge group, and so it must be completed in both cases.
333-
final CheckRun? lock;
334-
{
335-
final prBranch = pullRequest.head!.ref;
336-
if (isFusion && Config.defaultBranch(slug) == prBranch) {
337-
lock = await _lockMergeGroupChecks(slug, pullRequest.head!.sha!);
338-
} else {
339-
lock = null;
340-
log.debug('Skipping merge queue guard: $slug/$prBranch');
341-
}
342-
}
332+
final lock = await lockMergeGroupChecks(slug, pullRequest.head!.sha!);
343333

344-
// Track if we should unlock the merge group lock in case of rever bots.
345-
// This is different than simply not having a lock - because flutter/flutter
346-
// master requires to see the lock existing and have been passed (versus
347-
// non-existence).
334+
// Track if we should unlock the merge group lock in case of non-fusion or
335+
// revert bots.
348336
var unlockMergeGroup = false;
349337

350338
final ciValidationCheckRun = await _createCiYamlCheckRun(pullRequest, slug);
351339

352340
log.info('Creating presubmit targets for ${pullRequest.number}');
353341
Object? exception;
342+
final isFusion = slug == Config.flutterSlug;
354343
do {
355344
try {
356345
final sha = pullRequest.head!.sha!;
346+
if (!isFusion) {
347+
unlockMergeGroup = true;
348+
}
357349

358350
// Both the author and label should be checked to make sure that no one is
359351
// attempting to get a pull request without check through.
@@ -389,7 +381,7 @@ class Scheduler {
389381

390382
await _runCiTestingStage(
391383
pullRequest: pullRequest,
392-
checkRunGuard: lock?.toString() ?? '',
384+
checkRunGuard: '$lock',
393385
logCrumb: logCrumb,
394386

395387
// The if-branch already skips the engine build phase.
@@ -478,8 +470,8 @@ class Scheduler {
478470
// Normally the lock stays pending until the PR is ready to be enqueued, but
479471
// there are situations (see code above) when it needs to be unlocked
480472
// immediately.
481-
if (lock != null && unlockMergeGroup) {
482-
await _unlockMergeQueueGuard(slug, pullRequest.head!.sha!, lock);
473+
if (unlockMergeGroup) {
474+
await unlockMergeQueueGuard(slug, pullRequest.head!.sha!, lock);
483475
}
484476
log.info(
485477
'Finished triggering builds for: pr ${pullRequest.number}, commit ${pullRequest.base!.sha}, branch ${pullRequest.head!.ref} and slug $slug}',
@@ -596,12 +588,12 @@ class Scheduler {
596588
'triggerTargetsForMergeGroup($slug, $headSha, ${isFusion ? 'real' : 'simulated'})';
597589
log.info('$logCrumb: scheduling merge group checks');
598590

599-
final lock = await _lockMergeGroupChecks(slug, headSha);
591+
final lock = await lockMergeGroupChecks(slug, headSha);
600592

601593
// If the repo is not fusion, it doesn't run anything in the MQ, so just
602594
// close the merge group guard.
603595
if (!isFusion) {
604-
await _unlockMergeQueueGuard(slug, headSha, lock);
596+
await unlockMergeQueueGuard(slug, headSha, lock);
605597
return;
606598
}
607599

@@ -794,7 +786,7 @@ $s
794786
/// While this check is still in progress, the merge queue will not merge the
795787
/// respective PR onto the target branch (e.g. main or master), because this
796788
/// check is "required".
797-
Future<CheckRun> _lockMergeGroupChecks(
789+
Future<CheckRun> lockMergeGroupChecks(
798790
RepositorySlug slug,
799791
String headSha,
800792
) async {
@@ -818,7 +810,7 @@ $s
818810
///
819811
/// If the guard is guarding a pull request, this immediately makes the pull
820812
/// request eligible for enqueuing into the merge queue.
821-
Future<void> _unlockMergeQueueGuard(
813+
Future<void> unlockMergeQueueGuard(
822814
RepositorySlug slug,
823815
String headSha,
824816
CheckRun lock,
@@ -1177,11 +1169,7 @@ $s
11771169
required String logCrumb,
11781170
}) async {
11791171
log.info('$logCrumb: Stage completed: ${CiStage.fusionTests}');
1180-
await _unlockMergeQueueGuard(
1181-
slug,
1182-
sha,
1183-
checkRunFromString(mergeQueueGuard),
1184-
);
1172+
await unlockMergeQueueGuard(slug, sha, checkRunFromString(mergeQueueGuard));
11851173
}
11861174

11871175
/// Returns the presubmit targets for the fusion repo [pullRequest] that should run for the given [stage].
@@ -1214,7 +1202,7 @@ $s
12141202

12151203
// Unlock the guarding check_run.
12161204
final checkRunGuard = checkRunFromString(mergeQueueGuard);
1217-
await _unlockMergeQueueGuard(slug, sha, checkRunGuard);
1205+
await unlockMergeQueueGuard(slug, sha, checkRunGuard);
12181206
}
12191207

12201208
/// Schedules post-engine build tests (i.e. engine tests, and framework tests).

app_dart/test/service/scheduler_test.dart

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2285,6 +2285,11 @@ targets:
22852285
),
22862286
).captured,
22872287
<Object?>[
2288+
Config.kMergeQueueLockName,
2289+
const CheckRunOutput(
2290+
title: Config.kMergeQueueLockName,
2291+
summary: Scheduler.kMergeQueueLockDescription,
2292+
),
22882293
Config.kCiYamlCheckName,
22892294
const CheckRunOutput(
22902295
title: Config.kCiYamlCheckName,
@@ -2466,6 +2471,11 @@ targets:
24662471
),
24672472
).captured,
24682473
<Object?>[
2474+
Config.kMergeQueueLockName,
2475+
const CheckRunOutput(
2476+
title: Config.kMergeQueueLockName,
2477+
summary: Scheduler.kMergeQueueLockDescription,
2478+
),
24692479
Config.kCiYamlCheckName,
24702480
const CheckRunOutput(
24712481
title: Config.kCiYamlCheckName,
@@ -2502,6 +2512,11 @@ targets:
25022512
),
25032513
).captured,
25042514
<Object?>[
2515+
Config.kMergeQueueLockName,
2516+
const CheckRunOutput(
2517+
title: Config.kMergeQueueLockName,
2518+
summary: Scheduler.kMergeQueueLockDescription,
2519+
),
25052520
Config.kCiYamlCheckName,
25062521
const CheckRunOutput(
25072522
title: Config.kCiYamlCheckName,
@@ -2532,7 +2547,12 @@ targets:
25322547
output: anyNamed('output'),
25332548
),
25342549
).captured,
2535-
<Object?>[CheckRunStatus.completed, CheckRunConclusion.success],
2550+
<Object?>[
2551+
CheckRunStatus.completed,
2552+
CheckRunConclusion.success,
2553+
CheckRunStatus.completed,
2554+
CheckRunConclusion.success,
2555+
],
25362556
);
25372557
});
25382558

0 commit comments

Comments
 (0)