Skip to content

Commit b9bd02b

Browse files
Reverts "Break glass v2 (#4171)" (#4175)
Reverts: #4171 Initiated by: yjbanov Reason for reverting: the new code seems to be landing on red check runs Original PR Author: yjbanov Reviewed By: {jtmcdole} This change reverts the following previous change: Implement the new break glass behavior for the monorepo world. Fixes flutter/flutter#159898 Fixes flutter/flutter#132811 Below is an updated copy of flutter/flutter#159898 (comment): # Pull request ## Two-state Merge Queue Guard The guard can only be in two states: - **Pending** (yellow): this is the initial state, and the guard stays in this state for as long as the pull request is not allowed to enter the merge queue. - **Success** (green): the guard enters this state when the infra decides that the PR is allowed to enter the merge queue. The state can only go from pending to success. There are no other state transitions. ## Deciding when the guard goes green Going from pending to green is the only transition we need to worry about. This is how we decide it: - **Normal case**: the normal workflow for landing a PR is for all the checks to go green. Once this happens, Cocoon closes the guard (by making it green). This will allow the `autosubmit` label to enqueue the PR, and it will allow the author to press "Merge When Ready". - **Emergency case**: the a PR must be landed despite failing checks (typically on a red tree status). The author adds the `emergency` label. Cocoon immediately unlocks the guard, ignoring any pending or failed checks. In conjunction with the `autosubmit` label, Cocoon will also automatically enqueue the PR after all tests pass even if the tree status is red. With an `emergency` Cocoon will also let the PR to jump the queue. However, if the PR must be landed right away, the author can use the "Merge When Ready" button manually as soon as Cocoon unlocks the merge guard. ## Explainer The above system makes everything simpler. There are fewer states (just pending and success), fewer transitions (just pending => success), and fewer situations to consider (normal and emergency). We don't need to do anything special w.r.t. the guard for the purposes of retrying failed flaky tests. It simply stays pending while the author is doing whatever is necessary to make the PR green enough to land: push fixes, retry check runs, rebase, get approvals, etc. Importantly, it covers all situations we need to handle in PR management. ### Why not have a third "failed" state? The guard's job is not to tell you whether any tests are failing. You can already tell which tests are failing by looking at the status of respective check runs (this may change in the future, but when that time comes we'll find a new visual cue). The guard's job is only to tell you whether your PR is allowed to be enqueued. Permission to proceed never "fails". It's either granted (green), or not yet granted (pending). Once granted the PR is typically enqueued or landed immediately, so performing any other state transitions after green is mostly meaningless. Green is the final state of the guard, that's all. The other reason for keeping the guard in two states is for simpler state management. Once green, the PR can immediately be enqueued. There's no transition from pending to failed, from failed to pending, or from failed to green. We can remove some of the existing Cocoon code around this, and we don't have to add anything new to support normal and emergency PR landing. # Merge group There are two possible outcomes for a merge group: - **Land**: all check runs are green => Cocoon completes the merge guard as success and GitHub lands the merge group. - **Fail**: some check runs failed => Cocoon fails the merge guard and GitHub pulls the merge group (and the corresponding PR) out of the queue.
1 parent 932b91b commit b9bd02b

22 files changed

+450
-872
lines changed

app_dart/lib/src/model/firestore/ci_staging.dart

Lines changed: 5 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,7 @@ class CiStaging extends Document {
8888
required String checkRun,
8989
required String conclusion,
9090
}) async {
91-
final changeCrumb = '${slug.owner}_${slug.name}_$sha';
92-
final logCrumb = 'markConclusion(${changeCrumb}_$stage, $checkRun, $conclusion)';
91+
final logCrumb = 'markConclusion(${slug.owner}_${slug.name}_${sha}_$stage, $checkRun, $conclusion)';
9392

9493
// Marking needs to happen while in a transaction to ensure `remaining` is
9594
// updated correctly. For that to happen correctly; we need to perform a
@@ -111,10 +110,8 @@ class CiStaging extends Document {
111110

112111
var remaining = -1;
113112
var failed = -1;
114-
var total = -1;
115113
bool valid = false;
116114
String? checkRunGuard;
117-
String? recordedConclusion;
118115

119116
late final Document doc;
120117

@@ -149,15 +146,9 @@ class CiStaging extends Document {
149146
}
150147
failed = maybeFailed;
151148

152-
final maybeTotal = int.tryParse(fields[kTotalField]?.integerValue ?? '');
153-
if (maybeTotal == null) {
154-
throw '$logCrumb: missing field "$kTotalField" for $transaction / ${doc.fields}';
155-
}
156-
total = maybeTotal;
157-
158149
// We will have check_runs scheduled after the engine was built successfully, so missing the checkRun field
159150
// is an OK response to have. All fields should have been written at creation time.
160-
recordedConclusion = fields[checkRun]?.stringValue;
151+
final recordedConclusion = fields[checkRun]?.stringValue;
161152
if (recordedConclusion == null) {
162153
log.info('$logCrumb: $checkRun not present in doc for $transaction / $doc');
163154
await docRes.rollback(RollbackRequest(transaction: transaction), kDatabase);
@@ -166,8 +157,6 @@ class CiStaging extends Document {
166157
remaining: remaining,
167158
checkRunGuard: null,
168159
failed: failed,
169-
summary: 'Check run "$checkRun" not present in $stage CI stage',
170-
details: 'Change $changeCrumb',
171160
);
172161
}
173162

@@ -218,7 +207,7 @@ class CiStaging extends Document {
218207
fields[checkRun] = Value(stringValue: conclusion);
219208
fields[kRemainingField] = Value(integerValue: '$remaining');
220209
fields[kFailedField] = Value(integerValue: '$failed');
221-
} on DetailedApiRequestError catch (e, stack) {
210+
} on DetailedApiRequestError catch (e) {
222211
if (e.status == 404) {
223212
// An attempt to read a document not in firestore should not be retried.
224213
log.info('$logCrumb: staging document not found for $transaction');
@@ -228,15 +217,6 @@ class CiStaging extends Document {
228217
remaining: -1,
229218
checkRunGuard: null,
230219
failed: failed,
231-
summary: 'Internal server error',
232-
details: '''
233-
Staging document not found for CI stage "$stage" for $changeCrumb. Got 404 from
234-
Firestore.
235-
236-
Error:
237-
${e.toString()}
238-
$stack
239-
''',
240220
);
241221
}
242222
// All other errors should bubble up and be retried.
@@ -259,15 +239,6 @@ $stack
259239
remaining: remaining,
260240
checkRunGuard: checkRunGuard,
261241
failed: failed,
262-
summary: valid ? 'All tests passed' : 'Not a valid state transition for $checkRun',
263-
details: valid
264-
? '''
265-
For CI stage $stage:
266-
Total check runs scheduled: $total
267-
Pending: $remaining
268-
Failed: $failed
269-
'''
270-
: 'Attempted to transition the state of check run $checkRun from "$recordedConclusion" to "$conclusion".',
271242
);
272243
}
273244

@@ -372,16 +343,12 @@ class StagingConclusion {
372343
final int remaining;
373344
final String? checkRunGuard;
374345
final int failed;
375-
final String summary;
376-
final String details;
377346

378347
const StagingConclusion({
379348
required this.result,
380349
required this.remaining,
381350
required this.checkRunGuard,
382351
required this.failed,
383-
required this.summary,
384-
required this.details,
385352
});
386353

387354
bool get isOk => result == StagingConclusionResult.ok;
@@ -399,21 +366,8 @@ class StagingConclusion {
399366
other.result == result &&
400367
other.remaining == remaining &&
401368
other.checkRunGuard == checkRunGuard &&
402-
other.failed == failed &&
403-
other.summary == summary &&
404-
other.details == details);
405-
406-
@override
407-
int get hashCode => Object.hashAll([
408-
result,
409-
remaining,
410-
checkRunGuard,
411-
failed,
412-
summary,
413-
details,
414-
]);
369+
other.failed == failed);
415370

416371
@override
417-
String toString() =>
418-
'StagingConclusion("$result", "$remaining", "$checkRunGuard", "$failed", "$summary", "$details")';
372+
int get hashCode => Object.hashAll([result, remaining, checkRunGuard, failed]);
419373
}

app_dart/lib/src/service/config.dart

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -32,37 +32,11 @@ const String kDefaultBranchName = 'master';
3232
class Config {
3333
Config(this._db, this._cache);
3434

35-
/// When present on a pull request, instructs Cocoon to submit it
36-
/// automatically as soon as all the required checks pass.
35+
/// Labels autosubmit looks for on pull requests
3736
///
3837
/// Keep this in sync with the similar `Config` class in `auto_submit`.
3938
static const String kAutosubmitLabel = 'autosubmit';
4039

41-
/// When present on a pull request, allows it to land without passing all the
42-
/// checks, and jumps the merge queue.
43-
///
44-
/// Keep this in sync with the similar `Config` class in `auto_submit`.
45-
static const String kEmergencyLabel = 'emergency';
46-
47-
/// Validates that CI tasks were successfully created from the .ci.yaml file.
48-
///
49-
/// If this check fails, it means Cocoon failed to fully populate the list of
50-
/// CI checks and the PR/commit should be treated as failing.
51-
static const String kCiYamlCheckName = 'ci.yaml validation';
52-
53-
/// A required check that stays in pending state until a sufficient subset of
54-
/// checks pass.
55-
///
56-
/// This check is "required", meaning that it must pass before Github will
57-
/// allow a PR to land in the merge queue, or a merge group to land on the
58-
/// target branch (main or master).
59-
///
60-
/// IMPORTANT: the name of this task - "Merge Queue Guard" - must strictly
61-
/// match the name of the required check configured in the repo settings.
62-
/// Changing the name here or in the settings alone will break the PR
63-
/// workflow.
64-
static const String kMergeQueueLockName = 'Merge Queue Guard';
65-
6640
final DatastoreDB _db;
6741

6842
final CacheService _cache;
@@ -176,6 +150,7 @@ class Config {
176150

177151
// GitHub App properties.
178152
Future<String> get githubPrivateKey => _getSingleValue('githubapp_private_pem');
153+
Future<String> get overrideTreeStatusLabel => _getSingleValue('override_tree_status_label');
179154
Future<String> get githubPublicKey => _getSingleValue('githubapp_public_pem');
180155
Future<String> get githubAppId => _getSingleValue('githubapp_id');
181156
Future<Map<String, dynamic>> get githubAppInstallations async {

0 commit comments

Comments
 (0)