From 42d284bd7630b1b1d524cb7d0a619ac685853d29 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 27 May 2024 01:33:03 +0300 Subject: [PATCH 1/3] Automatically count required staging checks Now Anubis requests the list of required staging checks from GitHub 'stating branch' settings instead of maintaining its own config::staging_checks parameter. That parameter was problematic because it was usually out of sync with the actutal list of required checks. With new approach, the list of required staging checks should still be kept up-to-date. However, it became easier because it must include CI checks only (GitHub check runs are maintained automatically). --- README.md | 11 ++++--- src/Config.js | 2 -- src/MergeContext.js | 70 ++++++++++++++++++++++++++------------------- 3 files changed, 45 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 20e7190..d8a5f84 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,7 @@ are satisfied: branch is out of date with its target branch. * The PR is not a [draft](https://docs.github.com/en/github/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request). -* All the _required_ checks have succeeded on the PR branch: +* All the _required_ PR checks have succeeded on the PR branch: * If _all_ checks have succeeded, then GitHub says "All checks have passed" next to a green check mark: ![](./docs/images/all_passed.png) @@ -47,7 +47,7 @@ are satisfied: ![](./docs/images/required_passed.png) Unfortunately, GitHub also displays the above message when some - required checks have failed. To determine whether all the required + required PR checks have failed. To determine whether all the required PR checks have succeeded when some checks have failed, expand the check list using the "show all checks" link. * The PR is approved for merging (see below for voting rules). @@ -69,8 +69,8 @@ algorithm: section further below. 2. Reset the staging (a.k.a. "auto") branch to the PR staging commit. The previous state of the staging branch is ignored. -3. Wait for GitHub to report exactly `config::staging_checks` CI test - results for the staging branch. The project CI infrastructure is +3. Wait for GitHub to report that all the _required_ staging checks have + succeeded on the staging branch. The project CI infrastructure is expected to auto-test the staging branch on every change. A check failure is a PR-specific step failure -- in GitHub terminology, all staging checks are currently deemed "required". If discovered, any @@ -126,7 +126,7 @@ request state: for merging, successfully completed the initial merging steps that are supposed to trigger CI tests, and is now waiting for the CI test results. The bot removes this label when it notices that all - `config::staging_checks` tests have completed. + required staging checks have succeeded on the staging branch. * `M-passed-staging-checks`: Similar to the GitHub "green check" mark for the staging branch commit (but ignores failures of optional checks). Usually only visible when the bot is running in staging-only @@ -384,7 +384,6 @@ All configuration fields are required. *core_developers* | The comma-separated list of login=id pairs, specifying GitHub login and ID for core developers. | "" *voting_delay_min*| The minimum merging age of a PR. Younger PRs are not merged, regardless of the number of votes. The PR age string should comply with [timestring](https://github.com/mike182uk/timestring) parser. | "2d" *voting_delay_max* | The maximum merging age of a PR that has fewer than `config::sufficient_approvals` votes. The PR age string should comply with [timestring](https://github.com/mike182uk/timestring) parser. | "10d" -*staging_checks*| The expected number of CI tests executed against the staging branch. | 2 *approval_url*| The URL associated with an approval status test description. | "" *logger_params* | A JSON-formatted parameter list for the [Bunyan](https://github.com/trentm/node-bunyan) logging library [constructor](https://github.com/trentm/node-bunyan#constructor-api). | 
{
    "name": "anubis",
    "streams": [ ... ]
}
diff --git a/src/Config.js b/src/Config.js index e6d470b..e5e6883 100644 --- a/src/Config.js +++ b/src/Config.js @@ -29,7 +29,6 @@ class ConfigOptions { assert(this._sufficientApprovals > 1); this._votingDelayMax = timestring(conf.voting_delay_max, 'ms'); this._votingDelayMin = timestring(conf.voting_delay_min, 'ms'); - this._stagingChecks = conf.staging_checks; this._loggerParams = conf.logger_params; this._approvalUrl = conf.approval_url; this._coreDevelopers = conf.core_developers; @@ -96,7 +95,6 @@ class ConfigOptions { sufficientApprovals() { return this._sufficientApprovals; } votingDelayMax() { return this._votingDelayMax; } votingDelayMin() { return this._votingDelayMin; } - stagingChecks() { return this._stagingChecks; } loggerParams() { return this._loggerParams; } // returns a Map of (login,id) pairs coreDeveloperIds() { return this._coreDeveloperIds; } diff --git a/src/MergeContext.js b/src/MergeContext.js index 101ba27..c3d4efe 100644 --- a/src/MergeContext.js +++ b/src/MergeContext.js @@ -174,8 +174,7 @@ class StatusChecks // final() should count all statuses. // expectedStatusCount: - // for staged commits: the bot-configured number of required checks (Config.stagingChecks()), - // for PR commits: GitHub configured number of required checks (requested from GitHub) + // GitHub configured number of required checks (requested from GitHub) // context: either "PR" or "Staging"; // contextsRequiredByGitHubConfig: the GitHub protected branch checks in a format of a list of string contexts constructor(expectedStatusCount, context, contextsRequiredByGitHubConfig) { @@ -783,8 +782,13 @@ class PullRequest { this._approval = null; // future Approval object - // a list of proteced branch contexts received from GitHub - this._contextsRequiredByGitHubConfig = null; + // a list of proteced base branch contexts received from GitHub + // expects all required contexts including check runs + this._contextsRequiredByGitHubConfigBase = null; + + // a list of proteced staging branch contexts received from GitHub + // expects all required contexts other from check runs + this._contextsRequiredByGitHubConfigStaging = null; this._stagedPosition = null; @@ -942,11 +946,11 @@ class PullRequest { await GH.createStatus(sha, this._approval.state, Config.approvalUrl(), this._approval.description, Config.approvalContext()); } - async _loadRequiredContexts() { - assert(!this._contextsRequiredByGitHubConfig); + async _loadRequiredContexts(branch) { + let contextsRequiredByGitHubConfig = undefined; try { - this._contextsRequiredByGitHubConfig = await GH.getProtectedBranchRequiredStatusChecks(this._prBaseBranch()); + contextsRequiredByGitHubConfig = await GH.getProtectedBranchRequiredStatusChecks(branch); } catch (e) { if (e.name === 'ErrorContext' && e.notFound()) this._logEx(e, "no status checks are required"); @@ -954,36 +958,43 @@ class PullRequest { throw e; } - if (this._contextsRequiredByGitHubConfig === undefined) - this._contextsRequiredByGitHubConfig = []; + if (contextsRequiredByGitHubConfig === undefined) + contextsRequiredByGitHubConfig = []; - assert(this._contextsRequiredByGitHubConfig); - this._log("required contexts found: " + this._contextsRequiredByGitHubConfig.length); + assert(contextsRequiredByGitHubConfig); + this._log("required contexts found: " + contextsRequiredByGitHubConfig.length); + return contextsRequiredByGitHubConfig; + } + + async uniqueCheckRuns(sha) { + // Returns whole check runs history for the commit. + const checkRuns = await GH.getCheckRuns(sha); + // Filter out stale/older checks, assuming that check.id is greater in newer checks. + const sortedCheckRuns = checkRuns.sort((e1, e2) => parseInt(e2.id) - parseInt(e1.id)); + let uniqueCheckRuns = []; + sortedCheckRuns.forEach(check => { + if (!uniqueCheckRuns.some(e => e.name === check.name)) + uniqueCheckRuns.push(check); + }); + return uniqueCheckRuns; } // returns filled StatusChecks object async _getPrStatuses() { const combinedPrStatus = await GH.getStatuses(this._prHeadSha()); - let statusChecks = new StatusChecks(this._contextsRequiredByGitHubConfig.length, "PR", this._contextsRequiredByGitHubConfig); + let statusChecks = new StatusChecks(this._contextsRequiredByGitHubConfigBase.length, "PR", this._contextsRequiredByGitHubConfigBase); // fill with required status checks for (let st of combinedPrStatus.statuses) { - if (this._contextsRequiredByGitHubConfig.some(el => el.trim() === st.context.trim())) + if (this._contextsRequiredByGitHubConfigBase.some(el => el.trim() === st.context.trim())) statusChecks.addRequiredStatus(new StatusCheck(st)); else statusChecks.addOptionalStatus(new StatusCheck(st)); } - // Returns whole check runs history for the commit. - const checkRuns = await GH.getCheckRuns(this._prHeadSha()); - // Filter out stale/older checks, assuming that check.id is greater in newer checks. - const sortedCheckRuns = checkRuns.sort((e1, e2) => parseInt(e2.id) - parseInt(e1.id)); - let uniqueCheckRuns = []; - sortedCheckRuns.forEach(check => { - if (!uniqueCheckRuns.some(e => e.name === check.name)) - uniqueCheckRuns.push(check); - }); - for (let st of uniqueCheckRuns) { - if (this._contextsRequiredByGitHubConfig.some(el => el.trim() === st.name.trim())) + const checkRuns = await this.uniqueCheckRuns(this._prHeadSha()); + + for (let st of checkRuns) { + if (this._contextsRequiredByGitHubConfigBase.some(el => el.trim() === st.name.trim())) statusChecks.addRequiredStatus(StatusCheck.FromCheckRun(st)); else statusChecks.addOptionalStatus(StatusCheck.FromCheckRun(st)); @@ -997,8 +1008,8 @@ class PullRequest { async _getStagingStatuses() { const combinedStagingStatuses = await GH.getStatuses(this._stagedSha()); const genuineStatuses = combinedStagingStatuses.statuses.filter(st => !st.description.endsWith(Config.copiedDescriptionSuffix())); - assert(genuineStatuses.length <= Config.stagingChecks()); - let statusChecks = new StatusChecks(Config.stagingChecks(), "Staging", this._contextsRequiredByGitHubConfig); + const checkRuns = await this.uniqueCheckRuns(this._stagedSha()); // all check runs are 'required' + let statusChecks = new StatusChecks(this._contextsRequiredByGitHubConfigStaging.length + checkRuns.length, "Staging", this._contextsRequiredByGitHubConfigStaging); // all genuine checks, except for PR approval, are 'required' // PR approval may be either 'required' or 'optional' (depending on the GitHub settings) for (let st of genuineStatuses) { @@ -1016,8 +1027,6 @@ class PullRequest { statusChecks.addOptionalStatus(new StatusCheck(st)); } - // all check runs are 'required' - const checkRuns = await GH.getCheckRuns(this._stagedSha()); for (let st of checkRuns) statusChecks.addRequiredStatus(StatusCheck.FromCheckRun(st)); @@ -1449,7 +1458,7 @@ class PullRequest { async _supplyStagingWithPrRequired() { assert(this._stagedStatuses.succeeded()); - for (let requiredContext of this._contextsRequiredByGitHubConfig) { + for (let requiredContext of this._contextsRequiredByGitHubConfigBase) { if (this._stagedStatuses.hasStatus(requiredContext)) { this._log("_supplyStagingWithPrRequired: skip existing " + requiredContext); continue; @@ -1652,7 +1661,8 @@ class PullRequest { await this._loadStaged(); await this._loadRawPr(); // requires this._loadStaged() - await this._loadRequiredContexts(); + this._contextsRequiredByGitHubConfigBase = await this._loadRequiredContexts(this._prBaseBranch()); + this._contextsRequiredByGitHubConfigStaging = await this._loadRequiredContexts(Config.stagingBranch()); await this._loadPrState(); // requires this._loadRawPr(), this._loadRequiredContexts() and this._loadLabels() this._log("PR state: " + this._prState); From 61edc8276c0f1fed3e8f45743a592c07cb8fd4ba Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 28 May 2024 16:08:12 +0300 Subject: [PATCH 2/3] Remove expectedStatusCount parameter Anubis now expects that the staging branch protection includes the complete list of required checks. That list is extracted from GitHub (the same way as for PR itself) and the size of that list is the actual number of expected staging checks. --- README.md | 6 +----- src/MergeContext.js | 42 +++++++++++++++++++----------------------- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index d8a5f84..e8ad6aa 100644 --- a/README.md +++ b/README.md @@ -71,11 +71,7 @@ algorithm: The previous state of the staging branch is ignored. 3. Wait for GitHub to report that all the _required_ staging checks have succeeded on the staging branch. The project CI infrastructure is - expected to auto-test the staging branch on every change. A check - failure is a PR-specific step failure -- in GitHub terminology, all - staging checks are currently deemed "required". If discovered, any - extra check is considered a configuration problem (not related to any - specific PR). Error handling is detailed in the next section. + expected to auto-test the staging branch on every change. 4. Fast forward the PR target branch (usually "master") to the now-tested staging commit. 5. Label the PR as merged (see below for PR labels) and close the PR. diff --git a/src/MergeContext.js b/src/MergeContext.js index c3d4efe..e96bdac 100644 --- a/src/MergeContext.js +++ b/src/MergeContext.js @@ -173,16 +173,12 @@ class StatusChecks // For example, failed() should count only genuine statuses, whereas // final() should count all statuses. - // expectedStatusCount: - // GitHub configured number of required checks (requested from GitHub) // context: either "PR" or "Staging"; // contextsRequiredByGitHubConfig: the GitHub protected branch checks in a format of a list of string contexts - constructor(expectedStatusCount, context, contextsRequiredByGitHubConfig) { - assert(expectedStatusCount !== undefined); - assert(expectedStatusCount !== null); + constructor(context, contextsRequiredByGitHubConfig) { assert(context); assert(contextsRequiredByGitHubConfig); - this.expectedStatusCount = expectedStatusCount; + this._contextsRequiredByGitHubConfig = contextsRequiredByGitHubConfig; this.context = context; this._approvalIsRequired = contextsRequiredByGitHubConfig.some(el => el.trim() === Config.approvalContext()); this.requiredStatuses = []; @@ -233,7 +229,7 @@ class StatusChecks // no more required status changes or additions are expected final() { - return (this.requiredStatuses.length >= this.expectedStatusCount) && + return (this.requiredStatuses.length >= this._contextsRequiredByGitHubConfig.length) && this.requiredStatuses.every(check => !check.pending()); } @@ -255,7 +251,7 @@ class StatusChecks } toString() { - let combinedStatus = "context: " + this.context + " expected/required/optional: " + this.expectedStatusCount + "/" + + let combinedStatus = "context: " + this.context + " expected/required/optional: " + this._contextsRequiredByGitHubConfig.length + "/" + this.requiredStatuses.length + "/" + this.optionalStatuses.length + ", combined: "; if (this.failed()) combinedStatus += "failure"; @@ -783,11 +779,9 @@ class PullRequest { this._approval = null; // future Approval object // a list of proteced base branch contexts received from GitHub - // expects all required contexts including check runs this._contextsRequiredByGitHubConfigBase = null; // a list of proteced staging branch contexts received from GitHub - // expects all required contexts other from check runs this._contextsRequiredByGitHubConfigStaging = null; this._stagedPosition = null; @@ -982,13 +976,13 @@ class PullRequest { // returns filled StatusChecks object async _getPrStatuses() { const combinedPrStatus = await GH.getStatuses(this._prHeadSha()); - let statusChecks = new StatusChecks(this._contextsRequiredByGitHubConfigBase.length, "PR", this._contextsRequiredByGitHubConfigBase); - // fill with required status checks + let statusChecks = new StatusChecks("PR", this._contextsRequiredByGitHubConfigBase); for (let st of combinedPrStatus.statuses) { + const check = new StatusCheck(st); if (this._contextsRequiredByGitHubConfigBase.some(el => el.trim() === st.context.trim())) - statusChecks.addRequiredStatus(new StatusCheck(st)); + statusChecks.addRequiredStatus(check); else - statusChecks.addOptionalStatus(new StatusCheck(st)); + statusChecks.addOptionalStatus(check); } const checkRuns = await this.uniqueCheckRuns(this._prHeadSha()); @@ -1008,16 +1002,13 @@ class PullRequest { async _getStagingStatuses() { const combinedStagingStatuses = await GH.getStatuses(this._stagedSha()); const genuineStatuses = combinedStagingStatuses.statuses.filter(st => !st.description.endsWith(Config.copiedDescriptionSuffix())); - const checkRuns = await this.uniqueCheckRuns(this._stagedSha()); // all check runs are 'required' - let statusChecks = new StatusChecks(this._contextsRequiredByGitHubConfigStaging.length + checkRuns.length, "Staging", this._contextsRequiredByGitHubConfigStaging); - // all genuine checks, except for PR approval, are 'required' - // PR approval may be either 'required' or 'optional' (depending on the GitHub settings) + let statusChecks = new StatusChecks("Staging", this._contextsRequiredByGitHubConfigStaging); for (let st of genuineStatuses) { const check = new StatusCheck(st); - if (check.context.trim() === Config.approvalContext()) - statusChecks.setApprovalStatus(check); - else + if (this._contextsRequiredByGitHubConfigStaging.some(el => el.trim() === st.context.trim())) statusChecks.addRequiredStatus(check); + else + statusChecks.addOptionalStatus(check); } const optionalStatuses = combinedStagingStatuses.statuses.filter(st => st.description.endsWith(Config.copiedDescriptionSuffix())); @@ -1027,8 +1018,13 @@ class PullRequest { statusChecks.addOptionalStatus(new StatusCheck(st)); } - for (let st of checkRuns) - statusChecks.addRequiredStatus(StatusCheck.FromCheckRun(st)); + const checkRuns = await this.uniqueCheckRuns(this._stagedSha()); + for (let st of checkRuns) { + if (this._contextsRequiredByGitHubConfigStaging.some(el => el.trim() === st.name.trim())) + statusChecks.addRequiredStatus(StatusCheck.FromCheckRun(st)); + else + statusChecks.addOptionalStatus(StatusCheck.FromCheckRun(st)); + } this._log("staging status details: " + statusChecks); return statusChecks; From 36be6b1e58ae5ecf878b8dcff3dcbae1112cd016 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 28 May 2024 16:16:43 +0300 Subject: [PATCH 3/3] Removed a leftover --- config-example.json | 1 - 1 file changed, 1 deletion(-) diff --git a/config-example.json b/config-example.json index 0bb2737..f360d22 100644 --- a/config-example.json +++ b/config-example.json @@ -14,7 +14,6 @@ "sufficient_approvals": 2, "voting_delay_min": "2d", "voting_delay_max": "10d", - "staging_checks": 2, "approval_url": "", "logger_params" : { "name" : "anubis",