Keep PR GitHub metadata up-to-date #25

eduard-bagdasaryan · 2018-06-11T12:25:59Z

When reacting to an event, advance each open PR as much as possible instead of
waiting for the being-merged PR to fully merge before advancing the next PR.
At the minimum, update PR labels.

Keeping all PRs up-to-date required many architectural and implementation
fixes. Most of the "middle" code (somewhere between highest-level GitHub event
processing code and low-level GitHub operations) was rewritten.

Also remove stale PR tags of closed PRs.

Also fixed GH.getTags() which unexpectedly threw when getting an
empty tag list from GitHub.

With this change, before selecting a PR for processing, the bot updates PR metadata (such as labels, approvals, etc.) for every PR, except the current one. Updating the current PR was left as a TODO, because: * is not so important, since the PR has been adjusted already before initiation (i.e., it would not have been initiated without acceptable PR metadata). * it is not obvious how to implement (e.g., add another updater class?) because the PrUpdater class does not fit for this * the current PR would be most likely processed the first, and required adjustments would be performed anyway

Updater, Initiator and Finalizer classes were removed in favor of a new Merger class. This change is untested yet.

* Transformed MergeContext and Merger classes into a single PrProcessor. * Separated PR updating code from the general processing code, making possible to call PrProcessor.update() for all PRs (addressing 85dacc3 TODO). * Reworked PrProcessor.process() to return StepResult object (instead of inconvenient integer). Also fixed two bugs: * setTimeout() does not work properly for ms>2^32-1 (~24 days): in this case the value becomes 1ms (and the bot reruns again and again). Limit the maximum timeout value. * Always trim GitHub commit message(used in staged commits) before any usage(e.g., comparing) because by default GitHub does that trimming itself, when creating the commit.

rousskov

I found a couple of places that need further refactoring work. The rest is polishing.

src/MergeContext.js

rousskov · 2018-06-29T01:52:36Z

src/PrMerger.js

+                let pr = new PullRequest(rawPr);
+                const result = await pr.process(merging);
+                if (!merging)
+                    merging = result.succeeded();


This works, but I think the following form is more intuitive:

merging = merging || result.succeeded();

There is nothing intuitive about the succeeded() part in this context, but that will be changed to suspended() if other change requests are addressed. Suspended() makes more sense here, especially if we replace the too-specific merging with a general suspendedEarlier:

suspendedEarlier = suspendedEarlier || result.suspended();

rousskov · 2018-06-29T02:09:49Z

src/MergeContext.js

+    // Preconditions
+
+    // Check 'staging tag' state as merge condition.
+    // Returns true if there is a fresh merge commit with 'failure' status.


s/merge commit with 'failure' status/staged commit with failed status checks/

rousskov · 2018-06-29T02:11:32Z

src/MergeContext.js

+            return StepResult.Fail();
+        }
+
+        if (await this._stagingFailed()) {


This is not a precondition AFAICT -- staged PRs are handled after they are staged (naturally).

I renamed this method into _previoiusStagingFailed() to better reflect its purpose: this check is used to avoid looping over failed (but unchanged) PRs.

rousskov · 2018-06-29T02:16:34Z

src/MergeContext.js

-        this._log("checkApproval: " + this._approval);
-        await this._setApprovalStatus(this._prHeadSha());
-        await this._setApprovalStatus(this._tagSha);
-
        if (this._prInProgress()) {


Some conditions, like this one (if it is interpreted without peeking at its current implementation) and _prOpen() are both pre- and post-conditions. Have you tried grouping them into a dedicated _checkTimelessConditions() method that the other two _check*conditions() methods would call?

We should call _prInProgress() only after commits comparing so that we could track already merged cases (bot-unaware merges). Otherwise, the PR would be considered as 'failed', and restarts from scratch (though it should do only some cleanup and be closed then). So, for 'post' case we should not group _prOpen() and prInProgress().
We can group prOpen() and hasLabel('M-Merged') though.

I do not understand the parts of your your response talking about _prInProgress() -- AFAICT that method checks the WIP: prefix. Checking that PR title prefix has nothing to do with tracking already merged PRs. While it is currently recorded as a part of the title, the prefix should be treated like a label. I suggest s/InProgress/WorkInProgress/ or even s/prInProgress/wipPr/ to reduce clashes between "PR merging progress" and "work-in-progress PR".

I suggest s/InProgress/WorkInProgress/ or even s/prInProgress/wipPr/

Done.

rousskov · 2018-06-29T02:17:22Z

src/MergeContext.js

+    }
+
+    async _createStaged() {
+        this._log("start merging...");


s/merging/staging/

... thus addressing a related TODO.

With this change, two outcomes are meaningful for the caller: * suspended: the PR is in progress so the caller should not start a next PR * delayed: the PR is not ready but should be scheduled Other outcomes (succeeded() or failed()) mean only that this PR is not processed anymore (allowing to start another PR).

eduard-bagdasaryan

Done, b853ec7 is the last.

src/MergeContext.js

eduard-bagdasaryan · 2018-06-30T11:01:05Z

src/MergeContext.js

+    // Preconditions
+
+    // Check 'staging tag' state as merge condition.
+    // Returns true if there is a fresh merge commit with 'failure' status.


eduard-bagdasaryan · 2018-06-30T11:07:46Z

src/MergeContext.js

+            return StepResult.Fail();
+        }
+
+        if (await this._stagingFailed()) {


I renamed this method into _previoiusStagingFailed() to better reflect its purpose: this check is used to avoid looping over failed (but unchanged) PRs.

eduard-bagdasaryan · 2018-06-30T19:40:26Z

src/PrMerger.js

@@ -24,28 +22,32 @@ class PrMerger {
        return labels.find(lbl => lbl.name === Config.clearedForMergeLabel()) !== undefined;
    }

-    async _getPRList(finalizer) {
+    /// Obtain PR list from GitHub, updates GitHub PR state
+    /// (labels, approvals, etc.) and filters out PRs not ready


eduard-bagdasaryan · 2018-06-30T19:43:33Z

src/PrMerger.js

@@ -24,28 +22,32 @@ class PrMerger {
        return labels.find(lbl => lbl.name === Config.clearedForMergeLabel()) !== undefined;
    }

-    async _getPRList(finalizer) {
+    /// Obtain PR list from GitHub, updates GitHub PR state


eduard-bagdasaryan · 2018-06-30T20:08:16Z

src/MergeContext.js

+        if (running)
+            return result.delayed() ? result : StepResult.Fail();
+
+        if (await this._isStaging()) {


eduard-bagdasaryan · 2018-06-30T22:03:33Z

src/PrMerger.js

+                let pr = new PullRequest(rawPr);
+                const result = await pr.process(merging);
+                if (!merging)
+                    merging = result.succeeded();


eduard-bagdasaryan · 2018-06-30T22:05:01Z

src/MergeContext.js

-} // MergeFinalizer
+
+    // StepResult.Succeed: this PR is in-progress
+    // StepResult.Delay: this PR is delayed


rousskov

This is not a full review, but I added several comments that may lead to some code restructuring and a few polishing suggestions.

rousskov · 2018-07-01T00:48:15Z

src/MergeContext.js


-        await GH.addLabels(params);
+        if (await this._previousStagingFailed()) {
+            this._logFailedCondition("fresh merge commit with failed staging checks");


This parameter states the current/true condition. Most other callers state the expected condition (that failed).

Rewrote into "lack of fresh staging commit with failed checks".

rousskov · 2018-07-01T00:49:03Z

src/MergeContext.js

+            return StepResult.Fail();
+
+        if (!this._messageValid) {
+            this._logFailedCondition("commit message");


Let's say "valid commit message" to clarify that this parameter should document the expected condition (that failed).

rousskov · 2018-07-01T00:51:51Z

src/MergeContext.js

-        if (currentLabels.find(lbl => lbl.name === label) !== undefined) {
-            this._log("addLabel: skip already existing " + label);
-            return;
+    // checks whether this PR can be tagged


I do not think this description matches the code. Satisfying these conditions allows the bot to tag the PR, but that is not why the bot is checking these conditions. AFAICT, these are preconditions for staging, not just tagging. If tagging was the desired outcome, then the method would have skipped already tagged PRs. How about something like this: whether the PR should be staged (including re-staged)

rousskov · 2018-07-01T21:22:10Z

src/MergeContext.js

+            return StepResult.Fail();
+
+        if (this._prInProgress()) {
+            this._logFailedCondition("not in progress");


s/in progress/work-in-progress/

rousskov · 2018-07-01T21:29:15Z

src/MergeContext.js

@@ -952,10 +1007,49 @@ class MergeFinalizer extends MergeContext {
        return await this._cleanupMerged();

    }
-} // MergeFinalizer
+
+    // StepResult.Suspend: this PR is in-progress


Avoid "in-progress" term that means little and clashes with "work-in-progress" state. I doubt we need to (re)document StepResult states here. They should be (and probably are) documented where StepResult is declared. Here, we can use just them.

I would describe this method as

// Maintain Anubis-controlled PR metadata. // If possible, also merge or advance the PR towards merging. // The caller is responsible for setting PR labels computed here.

rousskov · 2018-07-01T21:47:28Z

src/MergeContext.js


-        if (await this._hasLabel(Config.mergedLabel(), this.prNumber())) {
-            this._logFailedCondition("already has merged status");
+        if (!(await this._isStaging())) {


GitHub does not let me mark that (unchanged) line below (number 880/888) but the "tagIsFresh()" condition seems out of place here. AFAICT, it should be a part of the "the PR is staged" check that is used as a guard for the staging and merging actions. Fixing this would allow to correctly re-stage stale PRs (during the staging action) instead of simply failing them here (during the merging action).

Though moving tagIsFresh() into isStaging() makes this 're-staging' improvement, as you noted, this can be dangerous for some (probably rare) scenarios when PR is 'half-merged'. For example, a PR can be merged to base (mergeToBase() completed successfully), but some of label cleanup failed (e.g. labelMerged()). In this case, the PR is still 'open', but already merged. After that, e.g., either base or PR branch changes so that the tag is not 'fresh' anymore (because the GitHub-generated merge commit changed). If we do as you suggest, the bot on its next iteration would start the PR from scratch. Without these changes, the bot does what is expected - it finishes the cleanup process because it comparers commits first and only after that calls tagIsFresh().

rousskov · 2018-07-01T21:51:13Z

src/MergeContext.js

-                return;
-            }
-            throw e;
+    async _checkTimelessConditions() {


Describe this check scope so that we know which checks belong here. I suspect the description would be similar to checks conditions shared by the staging and merging actions.

rousskov · 2018-07-01T22:03:12Z

src/MergeContext.js

-        this._log("checkApproval: " + this._approval);
-        await this._setApprovalStatus(this._prHeadSha());
-        await this._setApprovalStatus(this._tagSha);
-
        if (this._prInProgress()) {


I do not understand the parts of your your response talking about _prInProgress() -- AFAICT that method checks the WIP: prefix. Checking that PR title prefix has nothing to do with tracking already merged PRs. While it is currently recorded as a part of the title, the prefix should be treated like a label. I suggest s/InProgress/WorkInProgress/ or even s/prInProgress/wipPr/ to reduce clashes between "PR merging progress" and "work-in-progress PR".

rousskov · 2018-07-01T22:06:42Z

src/MergeContext.js

@@ -938,9 +941,61 @@ class MergeFinalizer extends MergeContext {
        }
    }

+    // Start processing


Since "processing" can be interpreted in many ways (metadata updates, staging, merging, staging+merging), I suggest removing these "Start processing" and "Finish processing" labels. Also, grouping methods together like this rarely works long-term. Sooner or later, some of the "start processing" methods will be added elsewhere.

src/RepoMerger.js

... addressing review remarks

eduard-bagdasaryan

Done at d19ba62.

eduard-bagdasaryan · 2018-07-02T08:29:10Z

src/MergeContext.js

-        this._log("checkApproval: " + this._approval);
-        await this._setApprovalStatus(this._prHeadSha());
-        await this._setApprovalStatus(this._tagSha);
-
        if (this._prInProgress()) {


I suggest s/InProgress/WorkInProgress/ or even s/prInProgress/wipPr/

Done.

eduard-bagdasaryan · 2018-07-02T08:40:58Z

src/MergeContext.js

@@ -938,9 +941,61 @@ class MergeFinalizer extends MergeContext {
        }
    }

+    // Start processing


eduard-bagdasaryan · 2018-07-02T08:55:57Z

src/MergeContext.js

@@ -952,10 +1007,49 @@ class MergeFinalizer extends MergeContext {
        return await this._cleanupMerged();

    }
-} // MergeFinalizer
+
+    // StepResult.Suspend: this PR is in-progress


src/RepoMerger.js

eduard-bagdasaryan · 2018-07-02T09:17:54Z

src/MergeContext.js

-                return;
-            }
-            throw e;
+    async _checkTimelessConditions() {


eduard-bagdasaryan · 2018-07-02T09:27:16Z

src/MergeContext.js


-        await GH.addLabels(params);
+        if (await this._previousStagingFailed()) {
+            this._logFailedCondition("fresh merge commit with failed staging checks");


Rewrote into "lack of fresh staging commit with failed checks".

eduard-bagdasaryan · 2018-07-02T09:28:57Z

src/MergeContext.js

+            return StepResult.Fail();
+
+        if (!this._messageValid) {
+            this._logFailedCondition("commit message");


eduard-bagdasaryan · 2018-07-02T09:31:02Z

src/MergeContext.js

@@ -855,25 +862,26 @@ class MergeFinalizer extends MergeContext {
        return false; // no staging-only mode by default
    }

-    async _checkConditions() {
-        const pr = await GH.getPR(this.prNumber(), true);
+    // checks whether this tagged PR can be merged


eduard-bagdasaryan · 2018-07-02T09:39:22Z

src/MergeContext.js


-        if (await this._hasLabel(Config.mergedLabel(), this.prNumber())) {
-            this._logFailedCondition("already has merged status");
+        if (!(await this._isStaging())) {


Removed this stale check.

eduard-bagdasaryan · 2018-07-02T10:50:03Z

src/MergeContext.js


-        if (await this._hasLabel(Config.mergedLabel(), this.prNumber())) {
-            this._logFailedCondition("already has merged status");
+        if (!(await this._isStaging())) {


Though moving tagIsFresh() into isStaging() makes this 're-staging' improvement, as you noted, this can be dangerous for some (probably rare) scenarios when PR is 'half-merged'. For example, a PR can be merged to base (mergeToBase() completed successfully), but some of label cleanup failed (e.g. labelMerged()). In this case, the PR is still 'open', but already merged. After that, e.g., either base or PR branch changes so that the tag is not 'fresh' anymore (because the GitHub-generated merge commit changed). If we do as you suggest, the bot on its next iteration would start the PR from scratch. Without these changes, the bot does what is expected - it finishes the cleanup process because it comparers commits first and only after that calls tagIsFresh().

The major change here is a new 'post-staged' processing state (in addition to 'pre-staged' and 'staged'), which starts just after successful fast-forwarding into the base branch. During this step all required PR cleanup should be done atomically. The difference between first two state and this new one is in that how errors/exceptions are handled. If there is an exception during pre-staged/staged, the higher-level code (PrMerger.js) just starts another PR. For post-staged exceptions, the PR is considered as 'suspended', and does not start another PR, until all required cleanup is completed. The PR state (and related information) is now calculated just after PR code gets control. This allowed to eliminate confusion and code duplication (e.g., with _tagIsFresh()). A special care is taken over possible 'half-cleanupped' PRs, when an exception occurs during post-staged. In this case, the code should aware that the requested PR list does not contain this PR and 'mergeable' status is not calculated for this PR.

rousskov

I have one or two potentially important questions (especially the one about suspending a PR that fails cleanup). The rest is polishing.

src/MergeContext.js

rousskov · 2018-07-04T04:51:23Z

src/MergeContext.js

+            await this._applyLabels();
+            throw e;
+        }
+        if (!this._prState.postStaged())


Why not apply labels unconditionally here? In other words, why is label application in post-staged state requires some special handling/protection?

This is done for cleanup atomicity purpose, as I described above. If we apply labels unconditionally here and an exception occurs while applying, this PR would be abandoned and cleanup would not finish.

It is not clear to me yet why an exception during applyLabels() would result in PR abandonment. Essentially the same concern is discussed inside _finalize() in my new review, so let's resolve that concern first. I am adding this comment (in an older review) just to remember to come back to this later.

src/MergeContext.js

rousskov · 2018-07-04T05:08:56Z

src/MergeContext.js

+        await this._loadPrState();
+        this._log("PR state calculated: " + this._prState.toString());
+        let result = await this.update();
+        if (anotherPrWasStaged)


Should we assert that update() never "suspends" the PR (i.e., never prevents another PR from being merged)?

src/MergeContext.js

rousskov · 2018-07-04T05:17:25Z

src/PrMerger.js

-            assert(result.succeeded());
-        }
-        return false;
-    }

    // Loads 'being-in-merge' PR, if exists (the PR has tag and staging_branch points to the tag).


I would clarify the intent along these lines:

// returns raw PR with a commit at the tip of the staging branch (or null) // if that PR exists, it is either "staged" or "post-staged"

src/MergeContext.js

eduard-bagdasaryan

Done at bfb516f.

src/MergeContext.js

eduard-bagdasaryan · 2018-07-04T08:58:10Z

src/MergeContext.js

+        this._log("PR state calculated: " + this._prState.toString());
+        let result = await this.update();
+        if (anotherPrWasStaged)
+            return result.delayed() ? result : StepResult.Fail();


We could return 'success' here as well: the caller does not care about these outcomes. However, I was thinking that this method returns 'success' if it managed to start/finish this PR and 'fail' in other cases (other from delaying). For example, it returns 'fail' if a PR condition failed. Similarly, we assume that 'anotherPrWasStaged' is a condition preventing the PR from start and thus should return 'fail'.

src/MergeContext.js

eduard-bagdasaryan · 2018-07-04T09:34:24Z

src/MergeContext.js

+            await this._applyLabels();
+            throw e;
+        }
+        if (!this._prState.postStaged())


This is done for cleanup atomicity purpose, as I described above. If we apply labels unconditionally here and an exception occurs while applying, this PR would be abandoned and cleanup would not finish.

eduard-bagdasaryan · 2018-07-04T09:55:33Z

src/PrMerger.js

-            assert(result.succeeded());
-        }
-        return false;
-    }

    // Loads 'being-in-merge' PR, if exists (the PR has tag and staging_branch points to the tag).


src/MergeContext.js

rousskov

You responses to the previous review were useful, but one or two related/overlapping concerns remain since the last review. The rest is minor polishing.

rousskov · 2019-01-06T23:47:06Z

src/MergeContext.js

@@ -4,7 +4,9 @@ const Log = require('./Logger.js');
 const GH = require('./GitHubUtil.js');
 const Util = require('./Util.js');

-// A result produced by MergeInitiator and MergeFinalizer classes
+// Action outcome, with support for paused and snoozed actions.
+// For example, is returned by PullRequest.process() (at higher level) or


Suggested change

// For example, is returned by PullRequest.process() (at higher level) or

// For example, returned by PullRequest.process() (at higher level) or

rousskov · 2019-01-07T00:11:13Z

src/MergeContext.js

+            await this._labels.apply();
+    }
+
+    // Atomize PR post-merge cleanup: suspend this PR


The description does not seem to match the code or is misleading -- we are not suspending a PR until its cleanup is complete; we suspend only after cleanup fails. The end result should be the same, but the sequence (and associated risks) are rather different.

The problem, in part, lies in the fact that we do not really suspend a PR here -- we only return the corresponding StepResult, but the PR state itself remains unchanged.

Another problem is that to make an operation (e.g., cleanup) "atomic" means that any partial result is undone during failures -- the operation either succeeds as a whole or has no side effects. This is not what happens here -- we undo nothing. This method does not make PR cleanup atomic.

I suggest the following description:

// Cleans up and closes a post-staged PR, removing it from our radar for good. // Returns "PR has been suspended" result on failures.

Adjust as needed based on the other change requests, of course.

rousskov · 2019-01-07T00:38:36Z

src/MergeContext.js

+            return StepResult.Succeed();
+        } catch (e) {
+            Log.LogException(e, this._toString() + " unexpected error while finalizing.");
+            return StepResult.Suspend();


From an earlier review:

Alex: Why suspend a PR that fails cleanup?

Eduard: The purpose is to guarantee PR consistency on GitHub. If the bot leaves a half-cleanupped PR on GitHub (and takes another one), it would not be able to fix this PR in future, because of its closed/merged attributes. So, if during cleanup there is an accidental GitHub-related error (which happen periodically), we just restart and finish cleanup as usual (after timeout). In case of a more serious bug, causing this exception, the bot should not pass to another PR either: it is better to do nothing in this case, until the bug is fixed.

I believe I understand what you are saying, but I am not sure I agree that special handling is actually desirable here. I may be missing some important detail about PR state. What are the flaws/mistakes in the following logic?

_finalize() deals with open post-staged tagged PRs. There are three cleanup steps:

If _applyLabels() fails, then the PR state does not change: The PR remains open, post-staged, and tagged. Thus, we can let _applyLables() throw and let the higher level code deal with that error as any other PR update failure (i.e., by eventually restarting the PR update process which will bring us back to the same _finalize() call). No special treatment seems to be needed to handle this step failure.

If closing the PR fails, then the PR state does not change: The PR remains open, post-staged, and tagged. Thus, we can let updatePR() throw and let the higher level code deal with that... (see step 1 for details).

If deleteReference() fails, then the PR is going to be closed/ignored but the tag will remain in the repository. I suspect that we should delete all such stale tags as a special high-level repository maintenance step (not dedicated to any specific PR). In fact, it may be easier to delete all stale tags in one location rather than delete them both here (under normal conditions; here they have been "stale" for just a moment since the previous updatePR() call) and then check for all truly stale tags in some higher-level code. We can just do the latter and, by removing this step, remove the need for special treatment from this code.

In summary, the first two update steps do not seem to need special handling while the third step should probably be moved to a central location (where it will stop being a "special" step). Am I missing something?

rousskov · 2019-01-07T00:49:20Z

src/MergeContext.js

+            await this._applyLabels();
+            throw e;
+        }
+        if (!this._prState.postStaged())


It is not clear to me yet why an exception during applyLabels() would result in PR abandonment. Essentially the same concern is discussed inside _finalize() in my new review, so let's resolve that concern first. I am adding this comment (in an older review) just to remember to come back to this later.

rousskov · 2019-01-07T01:00:06Z

src/MergeContext.js

+        let result = await this.update();
+        if (anotherPrWasStaged) {
+            assert(!result.suspended());
+            return result.delayed() ? result : StepResult.Fail();


Please always return result here. This was discussed in an earlier review, and I understand that you were trying to reserve success for PRs that were fully merged, but I do not think that translation is used/useful in the current code (AFAICT you agree with that), and it requires extra code (that confuses readers like me and would require a comment to define _doProcess() result).

Let's simplify by always returning result until the caller starts caring about the outcome (other than "suspended PR"). If that ever happens, we would know what the caller new needs are and will adjust this code accordingly.

rousskov · 2019-01-07T01:06:12Z

src/MergeContext.js


+        if (this._prState.postStaged()) {
+            this._role = "finalizer";
+            this._labelMerged();


This _labelMerged() operation should be a part of _finalize() method because that method is where we process postStaged() PRs. With other changes discussed in this review, it is possible that _finalize() will be called here, just like any other PR processing step, but even if _finalize() has to be called later/specially, I think this _labelMerged() call would belong there.

rousskov · 2019-01-07T01:08:54Z

src/RepoMerger.js

@@ -96,6 +96,10 @@ class RepoMerger {

    _plan(ms) {
        assert(ms > 0);
+        // obey node.js setTimeout() limits
+        const maxMs = Math.pow(2, 31) - 1;
+        ms = ms < maxMs ? ms : maxMs;


Suggested change

ms = ms < maxMs ? ms : maxMs;

const ms = Math.min(requestedMs, maxMs);

and rename the parameter to requestedMs, of course.

rousskov · 2019-01-07T01:14:30Z

src/MergeContext.js

+    }
+}
+
+// manage pull request states, one of:


Suggested change

// manage pull request states, one of:

// A state of a pull request (with regard to merging progress). One of:

Before this change, an error during PR finalizing (tagging, closing the PR or removing the PR tag) caused the PR suspending. At the next iteration this PR would become the current one, repeating all these steps (until all of them succeed); other PRs still wait. We gain two advantages with this improvement: * Error handling unification: an error at PR finalization is treated as errors at other steps (pre-staging or staging). * Better performance: instead of repeating finalizing the failed PR (which may fail again and again) we start the next PR as usual. The PR finalization will be completed once it becomes the current again. Finalization for closed (but still tagged) PRs is done specially, in a central location, where tags for all such PRs are deleted.

* Made _finalize() an explicit PR processing step call within doProcess(), like _stage() and _mergeStaged(). * Polished method descriptions and variable names.

* Do PR finalization while 'updating' PRs. This should help to cleanup PR finalization failures even if another PR is being staged. * Honor 'dry run' in _cleanTags(). * Optimize by filtering off PR-unrelated tags in _cleanTags(). * TagRegex should exactly match 'M-staged-PR...' name. * Methods description.

eduard-bagdasaryan

Done, the last change is a40e11.

eduard-bagdasaryan · 2019-01-16T21:14:03Z

src/MergeContext.js

@@ -4,7 +4,9 @@ const Log = require('./Logger.js');
 const GH = require('./GitHubUtil.js');
 const Util = require('./Util.js');

-// A result produced by MergeInitiator and MergeFinalizer classes
+// Action outcome, with support for paused and snoozed actions.
+// For example, is returned by PullRequest.process() (at higher level) or


eduard-bagdasaryan · 2019-01-16T21:14:28Z

src/MergeContext.js

+    }
+}
+
+// manage pull request states, one of:


eduard-bagdasaryan · 2019-01-16T21:15:19Z

src/MergeContext.js

+            await this._labels.apply();
+    }
+
+    // Atomize PR post-merge cleanup: suspend this PR


eduard-bagdasaryan · 2019-01-16T21:15:51Z

src/MergeContext.js

+            return StepResult.Succeed();
+        } catch (e) {
+            Log.LogException(e, this._toString() + " unexpected error while finalizing.");
+            return StepResult.Suspend();


eduard-bagdasaryan · 2019-01-16T21:16:37Z

src/MergeContext.js

+        let result = await this.update();
+        if (anotherPrWasStaged) {
+            assert(!result.suspended());
+            return result.delayed() ? result : StepResult.Fail();


eduard-bagdasaryan · 2019-01-16T21:16:53Z

src/MergeContext.js


+        if (this._prState.postStaged()) {
+            this._role = "finalizer";
+            this._labelMerged();


eduard-bagdasaryan · 2019-01-16T21:17:18Z

src/MergeContext.js

+            await this._applyLabels();
+            throw e;
+        }
+        if (!this._prState.postStaged())


eduard-bagdasaryan · 2019-01-16T21:17:32Z

src/RepoMerger.js

@@ -96,6 +96,10 @@ class RepoMerger {

    _plan(ms) {
        assert(ms > 0);
+        // obey node.js setTimeout() limits
+        const maxMs = Math.pow(2, 31) - 1;
+        ms = ms < maxMs ? ms : maxMs;


This fixes c633ff: we should finalize during update only non-current PRs, since the current PR is finalized at the post-staged step, as usual.

... by moving its stage-specific code into the corresponding ifs in doProcess().

It was addressed at 38574c.

rousskov

I found two places that may need non-trivial changes (look for two change requests one tied to _stagedCommitWillFail and one talking about the_tagIsFresh() description). The rest is trivial fixes/polishing.

src/MergeContext.js

src/PrMerger.js

This is needed for branches conflicts detection: the staged commit is obviously stale if there are conflicts (i.e., there were changes which caused these conflicts). Also some polishing and methods renaming.

... and set the label. Otherwise, the developer may miss this important information about broken PR code/tests.

See similar 1c819b change for PrMerger.

... thus avoiding unnecessary explicit setting this field.

Brewing PRs have nothing to resume on errors: such PRs are started from 'scratch' on any error.

eduard-bagdasaryan

Done in 1c819b-13d581.

src/PrMerger.js

src/MergeContext.js

rousskov · 2019-02-27T15:13:43Z

src/MergeContext.js

+    }
+}
+
+// promises to process a single PR at once, hiding PullRequest from callers


Suggested change

// promises to process a single PR at once, hiding PullRequest from callers

// promises to update/advance the given PR, hiding PullRequest from callers

rousskov · 2019-02-27T15:18:02Z

src/MergeContext.js

-        if (!this._prMergeable()) {
-            this._logFailedCondition("mergeable");
-            return StepResult.Fail();
+        // The staged commit became out of sync with PR and(or) base branches.


Please remove this comment -- the log message below it should be enough to [self-]document this code.

rousskov · 2019-02-27T15:19:02Z

src/MergeContext.js

-            return StepResult.Fail();
+        // The staged commit became out of sync with PR and(or) base branches.
+        if (!(await this._stagedCommitIsFresh())) {
+            this._log("the staged commit became stale due to PR branch and(or) base branch changes");


This log message became stale. I do not think we should enumerate all _stagedCommitIsFresh() checks here:

Suggested change

this._log("the staged commit became stale due to PR branch and(or) base branch changes");

this._log("the staged commit is stale");

src/MergeContext.js

rousskov

Thank you for addressing all my change requests. I think we are getting close to being done here, but should implement at least one of the TODOs you have added (see "commit message generation check") and discuss the other related TODO. The rest is minor polishing.

src/MergeContext.js

eduard-bagdasaryan

Done at 64f051.

src/MergeContext.js

eduard-bagdasaryan · 2019-02-27T22:06:23Z

src/MergeContext.js

-        if (!this._prMergeable()) {
-            this._logFailedCondition("mergeable");
-            return StepResult.Fail();
+        // The staged commit became out of sync with PR and(or) base branches.


eduard-bagdasaryan · 2019-02-27T22:07:28Z

src/MergeContext.js

-            return StepResult.Fail();
+        // The staged commit became out of sync with PR and(or) base branches.
+        if (!(await this._stagedCommitIsFresh())) {
+            this._log("the staged commit became stale due to PR branch and(or) base branch changes");


eduard-bagdasaryan · 2019-02-27T22:08:11Z

src/MergeContext.js

+    }
+}
+
+// promises to process a single PR at once, hiding PullRequest from callers


For example, we should not add "M-failed-staging-checks" if someone requested changes.

This is still a temporary solution: a complete solution should properly distinguish 'superficial' checks such as approvals from genuine checks, reported by CIs. Left that as a TODO.

Staged statues, loaded multiple times during a PR processing cycle may differ. We should avoid possible inconsistency when code decisions are based on different sets of staged statues.

The first(incomplete) step was done in a542cd for staged commit statuses. The similar logic is applicable to PR statuses. Since the statuses are loaded only once now, we need carefully maintain StatusChecks container after creating new approval statuses or copying PR statuses into staged commit statuses.

3b3f9e had a bug: we need to overwrite approval status instead of just copy it into the array.

rousskov

I cannot really track all the intricate dependencies in these latest changes, but I did not find any bugs. I inlined one polishing request. Please proceed with merging this when you are happy with the code.

rousskov · 2019-03-08T00:33:49Z

src/MergeContext.js

-
-        if (approvalStatus && this._approval.matchesGitHubStatusCheck(approvalStatus)) {
-            this._log("approval status already exists: " + Config.approvalContext() + ", " + this._approval);
+        if (this._dryRun("setting approval status"))


It looks like this check should be moved into _createApprovalStatus() because hasApprovalStatus() and addApprovalStatus() calls do not modify GitHub (and do allow us to get deeper in dry-run mode).

eduard-bagdasaryan added 5 commits June 11, 2018 15:00

Big refactoring, simplifying MergeContext hierarchy

a6ad4c0

Updater, Initiator and Finalizer classes were removed in favor of a new Merger class. This change is untested yet.

Renamed PrProcessor into PullRequest

910612d

Removed stale comments

5d1c03c

rousskov requested changes Jun 29, 2018

View reviewed changes

eduard-bagdasaryan added 4 commits June 30, 2018 15:46

Code polishing

f9759d2

Cache labels state and apply them only at the end of processing cycle

b1b948d

... thus addressing a related TODO.

Removed stale 'async' and polished b1b948d

18182df

eduard-bagdasaryan commented Jun 30, 2018

View reviewed changes

rousskov requested changes Jul 1, 2018

View reviewed changes

eduard-bagdasaryan added 2 commits July 2, 2018 13:59

Polished naming and comments

d19ba62

... addressing review remarks

Clarified some logging wording

85a89e2

eduard-bagdasaryan commented Jul 2, 2018

View reviewed changes

rousskov requested changes Jul 4, 2018

View reviewed changes

Polished comments and added several assertions

bfb516f

eduard-bagdasaryan commented Jul 4, 2018

View reviewed changes

rousskov requested changes Jan 7, 2019

View reviewed changes

eduard-bagdasaryan added 5 commits January 15, 2019 21:49

Further post-staged refactoring

4bae21a

* Made _finalize() an explicit PR processing step call within doProcess(), like _stage() and _mergeStaged(). * Polished method descriptions and variable names.

Merge branch 'master' into SQUID-330-merge-context-refactoring2

844e9e9

Fixed getTags(), when there are no tags on GitHub

a40e110

eduard-bagdasaryan commented Jan 16, 2019

View reviewed changes

eduard-bagdasaryan added 4 commits January 17, 2019 10:57

Do not call _finalize() twice for post-staged PRs

31497ed

This fixes c633ff: we should finalize during update only non-current PRs, since the current PR is finalized at the post-staged step, as usual.

Fixed a bug caused by 31497ed

3471b04

Simplified PullRequest.update()

3dc596e

... by moving its stage-specific code into the corresponding ifs in doProcess().

Further refactoring of PullRequest's update() and doProcess()

7b08745

Removed a stale TODO

e0a1595

It was addressed at 38574c.

rousskov requested changes Feb 26, 2019

View reviewed changes

rousskov reviewed Feb 26, 2019

View reviewed changes

src/MergeContext.js Outdated Show resolved Hide resolved

rousskov reviewed Feb 26, 2019

View reviewed changes

src/PrMerger.js Outdated Show resolved Hide resolved

eduard-bagdasaryan added 8 commits February 27, 2019 10:07

Redefined PrMerger.Step(), dropping async and returning a promise

1c819b3

Undone e0a159

ff7a926

Check also PR mergeable flag when determining staged commit freshness

56e6515

This is needed for branches conflicts detection: the staged commit is obviously stale if there are conflicts (i.e., there were changes which caused these conflicts). Also some polishing and methods renaming.

Check whether 'staged commit will fail' before other checks

58ee5be

... and set the label. Otherwise, the developer may miss this important information about broken PR code/tests.

Do not remove 'failed staged checks' label on staging failure

f8e73c0

Redefined MergeContext.Process(), dropping async and returning a promise

9e982a6

See similar 1c819b change for PrMerger.

ProcessResult: null _prStaged should mean that the PR is not staged

68bd47e

... thus avoiding unnecessary explicit setting this field.

Suspending brewing PRs has no sense

13d581b

Brewing PRs have nothing to resume on errors: such PRs are started from 'scratch' on any error.

eduard-bagdasaryan commented Feb 27, 2019

View reviewed changes

rousskov reviewed Feb 27, 2019

View reviewed changes

rousskov requested changes Feb 27, 2019

View reviewed changes

src/MergeContext.js Outdated Show resolved Hide resolved

src/MergeContext.js Outdated Show resolved Hide resolved

Polished method descriptions and comments

64f0513

eduard-bagdasaryan commented Feb 27, 2019

View reviewed changes

rousskov approved these changes Feb 28, 2019

View reviewed changes

eduard-bagdasaryan added 7 commits March 4, 2019 13:59

Missed 'await' at an async call

9e76c25

Simplified _stagedCommitIsFresh() and always debug its result

9581374

Treat failed approval status and failed checks statuses separately

52ce811

For example, we should not add "M-failed-staging-checks" if someone requested changes.

Ignore approval status when analysing failed checks

a007be6

This is still a temporary solution: a complete solution should properly distinguish 'superficial' checks such as approvals from genuine checks, reported by CIs. Left that as a TODO.

Load staged commit statues only once

a542cd8

Staged statues, loaded multiple times during a PR processing cycle may differ. We should avoid possible inconsistency when code decisions are based on different sets of staged statues.

Fixed a bug when adding approval statuses

9d6709d

3b3f9e had a bug: we need to overwrite approval status instead of just copy it into the array.

rousskov approved these changes Mar 8, 2019

View reviewed changes

Check for dryRun() just before creating statuses on GitHub

948a98c

eduard-bagdasaryan merged commit b485c4e into master Mar 10, 2019

This was referenced Mar 11, 2019

Initial merge bot implementation. measurement-factory/EggTimer#1

Closed

Initial merge bot implementation. measurement-factory/EggTimer#2

Closed

	// For example, is returned by PullRequest.process() (at higher level) or
	// For example, returned by PullRequest.process() (at higher level) or

	ms = ms < maxMs ? ms : maxMs;
	const ms = Math.min(requestedMs, maxMs);

	// manage pull request states, one of:
	// A state of a pull request (with regard to merging progress). One of:

	// promises to process a single PR at once, hiding PullRequest from callers
	// promises to update/advance the given PR, hiding PullRequest from callers

	this._log("the staged commit became stale due to PR branch and(or) base branch changes");
	this._log("the staged commit is stale");

Keep PR GitHub metadata up-to-date #25

Keep PR GitHub metadata up-to-date #25

Conversation

eduard-bagdasaryan commented Jun 11, 2018 • edited

rousskov 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

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

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

rousskov 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

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

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

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

rousskov 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

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

rousskov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan commented Jun 11, 2018 •

edited