Force waiting for automated merge #40

eduard-bagdasaryan · 2020-10-28T21:21:12Z

Each GitHub PR has a manual merge button on the GitHub PR page,
which becomes green as soon as all preconditions (mandatory PR checks)
are satisfied. At that time, some admins may click on that button,
resulting in wrong commit messages and missing auto tests (at best).

To reduce the probability of such accidents, Anubis now automatically
adds a "waiting for automated merge" check, marked as required on
GitHub.

For the staged commit, the check is satisfied immediately before
the attempted merge (so that GitHub is happy about all the required
status checks).
For the PR, the check is satisfied only after Anubis merges the
staged commit.

Each GitHub PR has a manual merge button on the GitHub PR page, which becomes green as soon as all preconditions (mandatory PR checks) are satisfied. At that time, some admins may click on that button, resulting in wrong commit messages and missing auto tests (at best). To reduce the probability of such accidents, Anubis now automatically adds a "waiting for automated merge" check, marked as required on GitHub. * For the staged commit, the check is satisfied immediately before the attempted merge (so that GitHub is happy about all the required status checks). * For the PR, the check is satisfied only after Anubis merges the staged commit.

rousskov · 2020-10-30T14:03:48Z

src/Config.js

+    automatedMergeStatusUrl() { return this._automatedMergeUrl; }
+
+    // the 'context name' of the automated merge test status
+    automatedMergeStatusContext() { return "Merge automatically"; }


Suggested change

automatedMergeStatusContext() { return "Merge automatically"; }

automatedMergeStatusContext() { return "Automated merge"; }

rousskov · 2020-10-30T14:08:18Z

src/MergeContext.js

+    }
+
+    async _setAutomatedMergeStatusForStaged(state) {
+        const description = this._automatedMergeStatusDescription();


Please declare closer to the first use.

rousskov · 2020-10-30T14:10:07Z

src/MergeContext.js

+        if (!Config.manageAutomatedMergeStatus())
+            return;
+
+        const state = this._prState.merged() ? "success" : "pending";


As we have discussed, this will become pending/success/failure.

rousskov · 2020-10-30T14:56:34Z

src/MergeContext.js

+                this._automatedMergeStatusDescription(), Config.automatedMergeStatusContext());
+    }
+
+    _automatedMergeStatusDescription() { return this._prState.toString(); }


As we have discussed, the automated merge status check description should be more detailed. We should also avoid internal/undocumented terminology like "brewing". Here are a few rough suggestions:

success (M-merged) and success (will be merged) -- see another change request for details.

failure (M-failed-foo) -- where M-failed-foo is a PR label that corresponds to a "problematic" condition "foo" that requires a human action to correct (e.g., M-failed-description). On a logical level, the set of such labels/conditions includes M-failed-review. There is no such label (yet?); we can use PR approval text instead. There can be multiple active M-failed-foo labels (at least in theory). If that can happen in the current code, to avoid noise and to simplify implementation, I would probably list just one label (e.g., the first encountered label or whichever is easier to implement).

pending (M-waiting-foo) -- where M-waiting-foo is a PR label that corresponds to a "positive" condition "foo" that Anubis is waiting for (e.g., M-waiting-staging-checks). On a logical level (at least), the set of such labels/conditions includes M-waiting-objections and M-waiting-approvals that are documented but not yet set; we can use their names (because they are documented) or just use PR approval text instead of those two label names (because they are not implemented). My weak recommendation is to do the latter. I do not think there can be multiple active M-waiting-foo labels (today), but please correct me if I am wrong.

When multiple states apply, the state listed higher (above) wins.

For any M-{waiting,failed}-staging-checks label inside the status check description, it would be nice to add a staged commit SHA: pending (M-waiting-staging-checks for SHA) and failure (M-failed-staging-checks for SHA).

Similarly, it would be nice to add SHA to the visible success status -- success (will be merged as SHA) and success (M-merged as SHA). AFAICT, success (will be merged as SHA) will be visible (at least for a while) if the final merge step fails so having SHA here would be useful. If the success (M-merged as SHA) status is never actually visible (except in the merged commit itself), then adding SHA to that specific status would be kind of pointless and, hence, is not necessary. On the other hand, automatic PR closure may fail or a human can reopen an auto-closed request, making even the "final" success status visible, right?

rousskov · 2020-10-30T15:08:11Z

src/MergeContext.js

+                el.context.trim() === Config.automatedMergeStatusContext() &&
+                el.state === state &&
+                el.description === description);


If possible, reorder to place faster and more selective conditions first:

Suggested change

el.context.trim() === Config.automatedMergeStatusContext() &&

el.state === state &&

el.description === description);

el.description === description &&

el.state === state &&

el.context.trim() === Config.automatedMergeStatusContext();

rousskov · 2020-10-30T15:26:37Z

src/MergeContext.js

@@ -1166,6 +1237,8 @@ class PullRequest {

        assert(!this._stagingBanned);

+        await this._setAutomatedMergeStatusForStaged("success");


Since we are setting this a bit prematurely/optimistically, it would be nice to make this one success (will merge SHA) and then, if the status may become visible anywhere after we successfully merge (as discussed in another change request), change that to success (M-merged as SHA).

including states not covered with labels, such as 'waiting for approval', 'work in progress', 'failed PR tests' and others. Also: * Utilize StatusCheck class when generating/creating automated checks * Removed all unnecessary status creations except for 'finally' block (and required one in_mergeToBase())

eduard-bagdasaryan

Done (28113c5, 3cfb8ba).

eduard-bagdasaryan · 2020-11-02T12:17:01Z

src/Config.js

+    automatedMergeStatusUrl() { return this._automatedMergeUrl; }
+
+    // the 'context name' of the automated merge test status
+    automatedMergeStatusContext() { return "Merge automatically"; }


eduard-bagdasaryan · 2020-11-02T12:21:54Z

src/MergeContext.js

+                el.context.trim() === Config.automatedMergeStatusContext() &&
+                el.state === state &&
+                el.description === description);


eduard-bagdasaryan · 2020-11-02T12:22:44Z

src/MergeContext.js

+    }
+
+    async _setAutomatedMergeStatusForStaged(state) {
+        const description = this._automatedMergeStatusDescription();


* When M-waiting-staging-checks is on * Just before merge on base

These statuses now take their description from the corresponding PrProblem context. I enhanced this context to keep additional information about labels and the status itself (pending/fail). Also improved the staged automated status description with additional information about the number succeeded/passed/failed tests.

This allowed to simplify StatusChecks code (also addressing a TODO) removing many additional checks and conditions related to special ('Approval' and 'Automated') statuses.

such as "PR" or "Staging".

I am not sure whether we can add footnotes at the end of a specific section - so it is better to avoid using them for now.

Fixed a couple of bugs revealed during testing. Also added Config.manage*() guards. Also calculate the number of derivative tests (instead of hardcoding them).

The abandoned staged commit is already marked with the corresponding 'Automated merge' status indicating this state. We do not need to duplicate this information in the label.

The underlying problem was that the DerivedStatusChecks object (tracking both Approval and Automated checks) could be missing at an early stage, i.e., before these checks had been requested from GitHub or manually created. Now we: * Ensure that DerivedStatusChecks exists when applying approvals (in other words, we ensure that we do not apply approvals at an early stage). * Do not update the automated check in this case. Since no 'known' error (aka PrProblem) can occur at this early stage (at least for now), it is probably better to consign these general (and probably PR-unrelated) errors to labels (such as M-failed-other), and update the automated check only after the problems are resolved.

This somehow relates to the second bullet of bd6e9ad. If we have got a PrProblem we must be sure that there is already the corresponding statuses object that we can update. Also removed some stale code and adjusted documentation regarding M-failed-staging-checks behavior.

This class is now a GitHub representation of the internal checks (containing cached values). When fresh internal checks are created, they are compared with the cached ones and applied only if there is a difference (still keeping the cache in sync).

eduard-bagdasaryan added 4 commits October 29, 2020 00:17

Adjusted documentation and a couple of fixes

4b6db52

Fixed an image

459ed6f

Polished documentation

3c01486

rousskov requested changes Oct 30, 2020

View reviewed changes

eduard-bagdasaryan added 2 commits November 2, 2020 18:03

Improved automated merge status reporting

28113c5

eduard-bagdasaryan commented Nov 3, 2020

View reviewed changes

eduard-bagdasaryan added 18 commits November 4, 2020 17:39

Limit SHA printing for automated merge status to two cases

bb28a22

* When M-waiting-staging-checks is on * Just before merge on base

Fixed _debugString() and some polishing

8002def

Distinguish CI-reported status checks from bot-reported ones

ab290cc

This allowed to simplify StatusChecks code (also addressing a TODO) removing many additional checks and conditions related to special ('Approval' and 'Automated') statuses.

Refactored CerivedStatusChecks to cover check for a specific scope

d9ccf2c

such as "PR" or "Staging".

Adjusted PrProblem messages to have similar style

5ecd6fd

Documentation adjustments and a couple of fixes

05b40c3

Removed footnotes from the documentation

882e936

I am not sure whether we can add footnotes at the end of a specific section - so it is better to avoid using them for now.

Bug fixing

ad34328

Fixed a couple of bugs revealed during testing. Also added Config.manage*() guards. Also calculate the number of derivative tests (instead of hardcoding them).

Code cleanup

374f0b6

Added an XXX

8dff72c

Refactored and adjusted documentation

d4da690

Fixed a couple of bugs and added debugging for each 'phase'

4262e17

Removed M-abandoned-staging-checks label

12bd7d8

The abandoned staged commit is already marked with the corresponding 'Automated merge' status indicating this state. We do not need to duplicate this information in the label.

Removed a stale XXX

03e483d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force waiting for automated merge #40

Force waiting for automated merge #40

eduard-bagdasaryan commented Oct 28, 2020

rousskov Oct 30, 2020

eduard-bagdasaryan Nov 2, 2020

rousskov Oct 30, 2020

eduard-bagdasaryan Nov 2, 2020

rousskov Oct 30, 2020

rousskov Oct 30, 2020

rousskov Oct 30, 2020

eduard-bagdasaryan Nov 2, 2020

rousskov Oct 30, 2020

eduard-bagdasaryan left a comment

eduard-bagdasaryan Nov 2, 2020

eduard-bagdasaryan Nov 2, 2020

eduard-bagdasaryan Nov 2, 2020

	automatedMergeStatusContext() { return "Merge automatically"; }
	automatedMergeStatusContext() { return "Automated merge"; }

		@@ -1166,6 +1237,8 @@ class PullRequest {

		assert(!this._stagingBanned);

		await this._setAutomatedMergeStatusForStaged("success");

Force waiting for automated merge #40

Are you sure you want to change the base?

Force waiting for automated merge #40

Conversation

eduard-bagdasaryan commented Oct 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment