diff --git a/README.md b/README.md index 20e7190..e8ad6aa 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,13 +69,9 @@ 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 - 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. +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. 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. @@ -126,7 +122,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 +380,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/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", 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..e96bdac 100644 --- a/src/MergeContext.js +++ b/src/MergeContext.js @@ -173,17 +173,12 @@ class StatusChecks // For example, failed() should count only genuine statuses, whereas // 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) // 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 = []; @@ -234,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()); } @@ -256,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,8 +778,11 @@ 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 + this._contextsRequiredByGitHubConfigBase = null; + + // a list of proteced staging branch contexts received from GitHub + this._contextsRequiredByGitHubConfigStaging = null; this._stagedPosition = null; @@ -942,11 +940,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,27 +952,17 @@ 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; } - // returns filled StatusChecks object - async _getPrStatuses() { - const combinedPrStatus = await GH.getStatuses(this._prHeadSha()); - let statusChecks = new StatusChecks(this._contextsRequiredByGitHubConfig.length, "PR", this._contextsRequiredByGitHubConfig); - // fill with required status checks - for (let st of combinedPrStatus.statuses) { - if (this._contextsRequiredByGitHubConfig.some(el => el.trim() === st.context.trim())) - statusChecks.addRequiredStatus(new StatusCheck(st)); - else - statusChecks.addOptionalStatus(new StatusCheck(st)); - } - + async uniqueCheckRuns(sha) { // Returns whole check runs history for the commit. - const checkRuns = await GH.getCheckRuns(this._prHeadSha()); + 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 = []; @@ -982,8 +970,25 @@ class PullRequest { 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())) + return uniqueCheckRuns; + } + + // returns filled StatusChecks object + async _getPrStatuses() { + const combinedPrStatus = await GH.getStatuses(this._prHeadSha()); + 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(check); + else + statusChecks.addOptionalStatus(check); + } + + 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,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())); - assert(genuineStatuses.length <= Config.stagingChecks()); - let statusChecks = new StatusChecks(Config.stagingChecks(), "Staging", this._contextsRequiredByGitHubConfig); - // 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())); @@ -1016,10 +1018,13 @@ 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)); + 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; @@ -1449,7 +1454,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 +1657,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);