Skip to content

fix: treat fast-track with not enough approvals as non-fatal #676

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.

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

Merged
merged 5 commits into from
Mar 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 18 additions & 12 deletions lib/pr_checker.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const WAIT_TIME_SINGLE_APPROVAL = 24 * 7;
const GITHUB_SUCCESS_CONCLUSIONS = ['SUCCESS', 'NEUTRAL', 'SKIPPED'];

const FAST_TRACK_RE = /^Fast-track has been requested by @(.+?)\. Please 👍 to approve\.$/;
const FAST_TRACK_MIN_APPROVALS = 2;
const GIT_CONFIG_GUIDE_URL = 'https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork';

export default class PRChecker {
Expand Down Expand Up @@ -138,7 +139,7 @@ export default class PRChecker {
const { requestedChanges, approved } = reviewers;
const labels = pr.labels.nodes.map((l) => l.name);

const isFastTracked = labels.includes('fast-track');
let isFastTracked = labels.includes('fast-track');
const isCodeAndLearn = labels.includes('code-and-learn');
const isSemverMajor = labels.includes('semver-major');

Expand Down Expand Up @@ -168,6 +169,7 @@ export default class PRChecker {
}
}

let fastTrackAppendix = '';
if (isFastTracked) {
const comment = [...this.comments].reverse().find((c) =>
FAST_TRACK_RE.test(c.bodyText));
Expand All @@ -183,14 +185,15 @@ export default class PRChecker {
r.user.login !== pr.author.login &&
collaborators.includes(r.user.login.toLowerCase())).length;

if (requester === pr.author.login && approvals < 2) {
cli.error('The fast-track request requires' +
" at least two collaborators' approvals (👍).");
return false;
} else if (approvals === 0) {
cli.error('The fast-track request requires' +
" at least one collaborator's approval (👍).");
return false;
const missingFastTrackApprovals = FAST_TRACK_MIN_APPROVALS - approvals -
(requester === pr.author.login ? 0 : 1);
if (missingFastTrackApprovals > 0) {
isFastTracked = false;
fastTrackAppendix = ' (or 0 hours if there ' +
`${missingFastTrackApprovals === 1 ? 'is' : 'are'} ` +
`${missingFastTrackApprovals} more approval` +
`${missingFastTrackApprovals === 1 ? '' : 's'} (👍) of ` +
'the fast-track request from collaborators).';
}
}

Expand All @@ -211,10 +214,12 @@ export default class PRChecker {
if (timeLeftMulti === 0) {
const timeLeftMins =
this.waitTimeMultiApproval * 60 - minutesFromCreateTime;
cli.error(`This PR needs to wait ${timeLeftMins} more minutes to land`);
cli.error(`This PR needs to wait ${timeLeftMins} ` +
`more minutes to land${fastTrackAppendix}`);
return false;
}
cli.error(`This PR needs to wait ${timeLeftMulti} more hours to land`);
cli.error(`This PR needs to wait ${timeLeftMulti} more ` +
`hours to land${fastTrackAppendix}`);
return false;
}

Expand All @@ -224,7 +229,8 @@ export default class PRChecker {
}
timeLeftMulti = timeLeftMulti < 0 || isFastTracked ? 0 : timeLeftMulti;
cli.error(`This PR needs to wait ${timeLeftSingle} more hours to land ` +
`(or ${timeLeftMulti} hours if there is one more approval)`);
`(or ${timeLeftMulti} hours if there is one more approval)` +
fastTrackAppendix);
return false;
}
}
Expand Down
55 changes: 51 additions & 4 deletions test/unit/pr_checker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -644,8 +644,9 @@ describe('PRChecker', () => {
[['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'],
['This PR is being fast-tracked']],
error:
[['The fast-track request requires at' +
" least two collaborators' approvals (👍)."]]
[['This PR needs to wait 24 more hours to land (or 0 hours if ' +
'there are 2 more approvals (👍) of the fast-track request from ' +
'collaborators).']]
};

const pr = Object.assign({}, firstTimerPR, {
Expand Down Expand Up @@ -694,8 +695,9 @@ describe('PRChecker', () => {
[['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'],
['This PR is being fast-tracked']],
error:
[['The fast-track request requires at' +
" least one collaborator's approval (👍)."]]
[['This PR needs to wait 24 more hours to land (or 0 hours if ' +
'there is 1 more approval (👍) of the fast-track request from ' +
'collaborators).']]
};

const pr = Object.assign({}, firstTimerPR, {
Expand Down Expand Up @@ -727,6 +729,51 @@ describe('PRChecker', () => {
cli.assertCalledWith(expectedLogs);
});

it('should error when not enough approvals or fast-track approvals', () => {
const cli = new TestCLI();

const expectedLogs = {
ok:
[['Approvals: 1'],
['- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624']],
info:
[['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'],
['This PR is being fast-tracked']],
error:
[['This PR needs to wait 144 more hours to land (or 24 hours if ' +
'there is one more approval) (or 0 hours if there is 1 more ' +
'approval (👍) of the fast-track request from collaborators).']]
};

const pr = Object.assign({}, firstTimerPR, {
createdAt: LT_48H,
labels: {
nodes: [
{ name: 'fast-track' }
]
}
});

const data = {
pr,
reviewers: singleGreenReviewer,
comments: commentsWithFastTrackInsuffientApprovals,
reviews: approvingReviews,
commits: [],
collaborators,
authorIsNew: () => true,
getThread() {
return PRData.prototype.getThread.call(this);
}
};
const checker = new PRChecker(cli, data, {}, argv);

cli.clearCalls();
const status = checker.checkReviewsAndWait(new Date(NOW));
assert(!status);
cli.assertCalledWith(expectedLogs);
});

it('should error when missing fast-track request comment', () => {
const cli = new TestCLI();

Expand Down