-
Notifications
You must be signed in to change notification settings - Fork 102
Break glass v2 #4171
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
Break glass v2 #4171
Conversation
app_dart/lib/src/service/config.dart
Outdated
| /// When present on a pull request, instructs Cocoon to land it automatically | ||
| /// as soon as all the required checks pass. |
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.
instead of "landing" which is what GitHub does; enqueued? submit pr?
jtmcdole
left a comment
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.
nits mostly
ef42de3 to
e584c31
Compare
jtmcdole
left a comment
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.
still lgtm
|
A reason for requesting a revert of flutter/cocoon/4171 could |
|
Reason for revert: the new code seems to be landing on red check runs |
This reverts commit 331f9c0.
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.
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:
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:
autosubmitlabel to enqueue the PR, and it will allow the author to press "Merge When Ready".emergencylabel. Cocoon immediately unlocks the guard, ignoring any pending or failed checks. In conjunction with theautosubmitlabel, Cocoon will also automatically enqueue the PR after all tests pass even if the tree status is red. With anemergencyCocoon 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: