Skip to content

Commit d5893f3

Browse files
mmarchinitargos
authored andcommitted
git-node: add config option to set wait times (#367)
Add two new config options: `waitTimeSingleApproval` and `waitTimeMultiApproval`, which can be used to set how long ncu should wait before landing a PR with 1 approval or with 2+ approvals.
1 parent def33eb commit d5893f3

5 files changed

Lines changed: 165 additions & 3 deletions

File tree

docs/git-node.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,12 @@ A custom Git command for managing pull requests. You can run it as
77
- [Prerequisites](#git-node-land-prerequisites)
88
- [Git bash for Windows](#git-bash-for-windows)
99
- [Demo & Usage](#demo--usage)
10+
- [Optional Settings](#git-node-land-optional-settings)
1011
- [`git node backport`](#git-node-backport)
1112
- [Example](#example)
1213
- [`git node sync`](#git-node-sync)
1314
- [`git node metadata`](#git-node-metadata)
15+
- [Optional Settings](#git-node-metadata-optional-settings)
1416
- [`git node v8`](#git-node-v8)
1517
- [Prerequisites](#git-node-v8-prerequisites)
1618
- [`git node v8 major`](#git-node-v8-major)
@@ -134,6 +136,14 @@ Options:
134136
--help Show help [boolean]
135137
```
136138

139+
<a id="git-node-land-optional-settings"></a>
140+
141+
### Optional Settings
142+
143+
The same Settings used by
144+
[`git node metadata`](#git-node-metadata-optional-settings) are also used by
145+
`git node land`.
146+
137147
## `git node backport`
138148

139149
Demo: https://asciinema.org/a/221244
@@ -230,6 +240,24 @@ $ git commit --amend -F msg.txt
230240
git node metadata 167 --repo llnode --readme ../node/README.md
231241
```
232242

243+
<a id="git-node-metadata-optional-settings"></a>
244+
245+
### Optional Settings
246+
247+
Some projects might not follow the same rules as nodejs/node. To properly
248+
validate Pull Requests for these projects, node-core-utils accept the following
249+
optional settings:
250+
251+
```bash
252+
cd path/to/project
253+
# waitTimeSingleApproval is the minimum wait time (in hours) before
254+
# landing a PR with only one approval. Default to 7 days.
255+
ncu-config set waitTimeSingleApproval 168
256+
# waitTimeMultiApproval is the minimum wait time (in hours) before
257+
# landing a PR with only two or more approvals. Default to 48 hours.
258+
ncu-config set waitTimeMultiApproval 48
259+
```
260+
233261
## `git node v8`
234262

235263
Update or patch the V8 engine.

lib/pr_checker.js

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,20 @@ class PRChecker {
5252
);
5353
}
5454

55+
get waitTimeSingleApproval() {
56+
if (this.argv.waitTimeSingleApproval === undefined) {
57+
return WAIT_TIME_SINGLE_APPROVAL;
58+
}
59+
return this.argv.waitTimeSingleApproval;
60+
}
61+
62+
get waitTimeMultiApproval() {
63+
if (this.argv.waitTimeMultiApproval === undefined) {
64+
return WAIT_TIME_MULTI_APPROVAL;
65+
}
66+
return this.argv.waitTimeMultiApproval;
67+
}
68+
5569
checkAll(checkComments = false) {
5670
const status = [
5771
this.checkCommitsAfterReview(),
@@ -151,8 +165,8 @@ class PRChecker {
151165
const msFromCreateTime = now.getTime() - createTime.getTime();
152166
const minutesFromCreateTime = Math.ceil(msFromCreateTime / MINUTE);
153167
const hoursFromCreateTime = Math.ceil(msFromCreateTime / HOUR);
154-
let timeLeftMulti = WAIT_TIME_MULTI_APPROVAL - hoursFromCreateTime;
155-
const timeLeftSingle = WAIT_TIME_SINGLE_APPROVAL - hoursFromCreateTime;
168+
let timeLeftMulti = this.waitTimeMultiApproval - hoursFromCreateTime;
169+
const timeLeftSingle = this.waitTimeSingleApproval - hoursFromCreateTime;
156170

157171
if (approved.length >= 2) {
158172
if (isFastTracked || isCodeAndLearn) {
@@ -163,7 +177,7 @@ class PRChecker {
163177
}
164178
if (timeLeftMulti === 0) {
165179
const timeLeftMins =
166-
WAIT_TIME_MULTI_APPROVAL * 60 - minutesFromCreateTime;
180+
this.waitTimeMultiApproval * 60 - minutesFromCreateTime;
167181
cli.error(`This PR needs to wait ${timeLeftMins} more minutes to land`);
168182
return false;
169183
}

lib/session.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ class Session {
5353
upstream: this.upstream,
5454
branch: this.branch,
5555
readme: this.readme,
56+
waitTimeSingleApproval: this.waitTimeSingleApproval,
57+
waitTimeMultiApproval: this.waitTimeMultiApproval,
5658
prid: this.prid
5759
};
5860
}
@@ -81,6 +83,14 @@ class Session {
8183
return this.config.readme;
8284
}
8385

86+
get waitTimeSingleApproval() {
87+
return this.config.waitTimeSingleApproval;
88+
}
89+
90+
get waitTimeMultiApproval() {
91+
return this.config.waitTimeMultiApproval;
92+
}
93+
8494
get pullName() {
8595
return `${this.owner}/${this.repo}/pulls/${this.prid}`;
8696
}

test/fixtures/data.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ const requestedChangesReviewers = {
2424
approved: []
2525
};
2626

27+
const noReviewers = {
28+
requestedChanges: [],
29+
approved: []
30+
};
31+
2732
const approvingReviews = readJSON('reviews_approved.json');
2833
const requestingChangesReviews = readJSON('reviews_requesting_changes.json');
2934

@@ -87,6 +92,7 @@ module.exports = {
8792
requestedChanges,
8893
allGreenReviewers,
8994
singleGreenReviewer,
95+
noReviewers,
9096
requestedChangesReviewers,
9197
approvingReviews,
9298
requestingChangesReviews,

test/unit/pr_checker.test.js

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ const {
2020
requestedChangesReviewers,
2121
approvingReviews,
2222
requestingChangesReviews,
23+
noReviewers,
2324
commentsWithCI,
2425
commentsWithLiteCI,
2526
commentsWithLGTM,
@@ -217,6 +218,42 @@ describe('PRChecker', () => {
217218
cli.assertCalledWith(expectedLogs);
218219
});
219220

221+
it('should succeed if waitTimeMultiApproval is set', () => {
222+
const cli = new TestCLI();
223+
224+
const expectedLogs = {
225+
ok:
226+
[ [ 'Approvals: 4' ],
227+
[ '- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624' ],
228+
[ '- Quux User (@Quux): LGTM' ],
229+
[ '- Baz User (@Baz): https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236' ],
230+
[ '- Bar User (@bar) (TSC): lgtm' ] ],
231+
info: [ [ 'This PR was created on Fri, 30 Nov 2018 17:50:44 GMT' ] ]
232+
};
233+
234+
const youngPR = Object.assign({}, firstTimerPR, {
235+
createdAt: LT_48H
236+
});
237+
238+
const data = {
239+
pr: youngPR,
240+
reviewers: allGreenReviewers,
241+
comments: commentsWithLGTM,
242+
reviews: approvingReviews,
243+
commits: simpleCommits,
244+
collaborators,
245+
authorIsNew: () => true,
246+
getThread() {
247+
return PRData.prototype.getThread.call(this);
248+
}
249+
};
250+
const checker = new PRChecker(cli, data, { waitTimeMultiApproval: 23 });
251+
252+
const status = checker.checkReviewsAndWait(new Date(NOW));
253+
assert(status);
254+
cli.assertCalledWith(expectedLogs);
255+
});
256+
220257
it('should error when PR is younger than 48h and older than 47h', () => {
221258
const cli = new TestCLI();
222259

@@ -326,6 +363,73 @@ describe('PRChecker', () => {
326363
cli.assertCalledWith(expectedLogs);
327364
});
328365

366+
it('should succeed with 1 approval with waitTimeSingleApproval set', () => {
367+
const cli = new TestCLI();
368+
369+
const expectedLogs = {
370+
ok:
371+
[ [ 'Approvals: 1' ],
372+
[ '- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624' ] ],
373+
info:
374+
[ [ 'This PR was created on Fri, 30 Nov 2018 17:50:44 GMT' ] ]
375+
};
376+
377+
const youngPR = Object.assign({}, firstTimerPR, {
378+
createdAt: LT_48H
379+
});
380+
381+
const data = {
382+
pr: youngPR,
383+
reviewers: singleGreenReviewer,
384+
comments: commentsWithLGTM,
385+
reviews: approvingReviews,
386+
commits: simpleCommits,
387+
collaborators,
388+
authorIsNew: () => true,
389+
getThread() {
390+
return PRData.prototype.getThread.call(this);
391+
}
392+
};
393+
const checker = new PRChecker(cli, data, { waitTimeSingleApproval: 0 });
394+
395+
const status = checker.checkReviewsAndWait(new Date(NOW));
396+
assert(status);
397+
cli.assertCalledWith(expectedLogs);
398+
});
399+
400+
it('should error with 0 approval and waitTimeSingleApproval=0', () => {
401+
const cli = new TestCLI();
402+
403+
const expectedLogs = {
404+
info:
405+
[ [ 'This PR was created on Fri, 30 Nov 2018 17:50:44 GMT' ] ],
406+
error:
407+
[ [ 'Approvals: 0' ] ]
408+
};
409+
410+
const youngPR = Object.assign({}, firstTimerPR, {
411+
createdAt: LT_48H
412+
});
413+
414+
const data = {
415+
pr: youngPR,
416+
reviewers: noReviewers,
417+
comments: commentsWithLGTM,
418+
reviews: approvingReviews,
419+
commits: simpleCommits,
420+
collaborators,
421+
authorIsNew: () => true,
422+
getThread() {
423+
return PRData.prototype.getThread.call(this);
424+
}
425+
};
426+
const checker = new PRChecker(cli, data, { waitTimeSingleApproval: 0 });
427+
428+
const status = checker.checkReviewsAndWait(new Date(NOW));
429+
assert(!status);
430+
cli.assertCalledWith(expectedLogs);
431+
});
432+
329433
it('should log as expected if PR can be fast-tracked', () => {
330434
const cli = new TestCLI();
331435

0 commit comments

Comments
 (0)