Skip to content
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.

Sign up for GitHub

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

Automatically count required staging checks #68

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Automatically count required staging checks #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42d284b
Automatically count required staging checks
eduard-bagdasaryan May 26, 2024
61edc82
Remove expectedStatusCount parameter
eduard-bagdasaryan May 28, 2024
36be6b1
Removed a leftover
eduard-bagdasaryan May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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).
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now all 'extra' checks will be treated as optional (i.e., the checks that are not 'required').

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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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). | <pre>{<br> "name": "anubis",<br> "streams": [ ... ]<br>}</pre>

Expand Down
1 change: 0 additions & 1 deletion config-example.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
"sufficient_approvals": 2,
"voting_delay_min": "2d",
"voting_delay_max": "10d",
"staging_checks": 2,
"approval_url": "",
"logger_params" : {
"name" : "anubis",
Expand Down
2 changes: 0 additions & 2 deletions src/Config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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; }
Expand Down
98 changes: 52 additions & 46 deletions src/MergeContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];
Expand Down Expand Up @@ -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) &&
Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.requiredStatuses.length should be <= this._contextsRequiredByGitHubConfig.length because this.requiredStatuses gets only those statuses that are among this._contextsRequiredByGitHubConfig. So we can probably assert the '<=' condition here and use '==' instead for the returning condition.

this.requiredStatuses.every(check => !check.pending());
}

Expand All @@ -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";
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -942,48 +940,55 @@ 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");
else
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 = [];
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()))
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));
Expand All @@ -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()));
Expand All @@ -1016,10 +1018,13 @@ class PullRequest {
statusChecks.addOptionalStatus(new StatusCheck(st));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_getPrStatuses() and _getStagingStatuses() look almost the same now, except for this dealing with 'copied' statuses (although the diff does not show it clearly).


// 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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down