diff --git a/docs/git-node.md b/docs/git-node.md index 68bd6ffb..69b5b3c6 100644 --- a/docs/git-node.md +++ b/docs/git-node.md @@ -7,10 +7,12 @@ A custom Git command for managing pull requests. You can run it as - [Prerequisites](#git-node-land-prerequisites) - [Git bash for Windows](#git-bash-for-windows) - [Demo & Usage](#demo--usage) + - [Optional Settings](#git-node-land-optional-settings) - [`git node backport`](#git-node-backport) - [Example](#example) - [`git node sync`](#git-node-sync) - [`git node metadata`](#git-node-metadata) + - [Optional Settings](#git-node-metadata-optional-settings) - [`git node v8`](#git-node-v8) - [Prerequisites](#git-node-v8-prerequisites) - [`git node v8 major`](#git-node-v8-major) @@ -134,6 +136,14 @@ Options: --help Show help [boolean] ``` + + +### Optional Settings + +The same Settings used by +[`git node metadata`](#git-node-metadata-optional-settings) are also used by +`git node land`. + ## `git node backport` Demo: https://asciinema.org/a/221244 @@ -230,6 +240,24 @@ $ git commit --amend -F msg.txt git node metadata 167 --repo llnode --readme ../node/README.md ``` + + +### Optional Settings + +Some projects might not follow the same rules as nodejs/node. To properly +validate Pull Requests for these projects, node-core-utils accept the following +optional settings: + +```bash +cd path/to/project +# waitTimeSingleApproval is the minimum wait time (in hours) before +# landing a PR with only one approval. Default to 7 days. +ncu-config set waitTimeSingleApproval 168 +# waitTimeMultiApproval is the minimum wait time (in hours) before +# landing a PR with only two or more approvals. Default to 48 hours. +ncu-config set waitTimeMultiApproval 48 +``` + ## `git node v8` Update or patch the V8 engine. diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 815c7961..ddeb127a 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -49,6 +49,20 @@ class PRChecker { ); } + get waitTimeSingleApproval() { + if (this.argv.waitTimeSingleApproval === undefined) { + return WAIT_TIME_SINGLE_APPROVAL; + } + return this.argv.waitTimeSingleApproval; + } + + get waitTimeMultiApproval() { + if (this.argv.waitTimeMultiApproval === undefined) { + return WAIT_TIME_MULTI_APPROVAL; + } + return this.argv.waitTimeMultiApproval; + } + checkAll(checkComments = false) { const status = [ this.checkCommitsAfterReview(), @@ -148,8 +162,8 @@ class PRChecker { const msFromCreateTime = now.getTime() - createTime.getTime(); const minutesFromCreateTime = Math.ceil(msFromCreateTime / MINUTE); const hoursFromCreateTime = Math.ceil(msFromCreateTime / HOUR); - let timeLeftMulti = WAIT_TIME_MULTI_APPROVAL - hoursFromCreateTime; - const timeLeftSingle = WAIT_TIME_SINGLE_APPROVAL - hoursFromCreateTime; + let timeLeftMulti = this.waitTimeMultiApproval - hoursFromCreateTime; + const timeLeftSingle = this.waitTimeSingleApproval - hoursFromCreateTime; if (approved.length >= 2) { if (isFastTracked || isCodeAndLearn) { @@ -160,7 +174,7 @@ class PRChecker { } if (timeLeftMulti === 0) { const timeLeftMins = - WAIT_TIME_MULTI_APPROVAL * 60 - minutesFromCreateTime; + this.waitTimeMultiApproval * 60 - minutesFromCreateTime; cli.error(`This PR needs to wait ${timeLeftMins} more minutes to land`); return false; } diff --git a/lib/session.js b/lib/session.js index 0d6d1dc8..a516a85b 100644 --- a/lib/session.js +++ b/lib/session.js @@ -50,6 +50,8 @@ class Session { upstream: this.upstream, branch: this.branch, readme: this.readme, + waitTimeSingleApproval: this.waitTimeSingleApproval, + waitTimeMultiApproval: this.waitTimeMultiApproval, prid: this.prid }; } @@ -78,6 +80,14 @@ class Session { return this.config.readme; } + get waitTimeSingleApproval() { + return this.config.waitTimeSingleApproval; + } + + get waitTimeMultiApproval() { + return this.config.waitTimeMultiApproval; + } + get pullName() { return `${this.owner}/${this.repo}/pulls/${this.prid}`; } diff --git a/test/fixtures/data.js b/test/fixtures/data.js index 678d85f1..3ca3e42f 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -24,6 +24,11 @@ const requestedChangesReviewers = { approved: [] }; +const noReviewers = { + requestedChanges: [], + approved: [] +}; + const approvingReviews = readJSON('reviews_approved.json'); const requestingChangesReviews = readJSON('reviews_requesting_changes.json'); @@ -87,6 +92,7 @@ module.exports = { requestedChanges, allGreenReviewers, singleGreenReviewer, + noReviewers, requestedChangesReviewers, approvingReviews, requestingChangesReviews, diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index b78618e8..595a18ac 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -20,6 +20,7 @@ const { requestedChangesReviewers, approvingReviews, requestingChangesReviews, + noReviewers, commentsWithCI, commentsWithLiteCI, commentsWithLGTM, @@ -217,6 +218,42 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should succeed if waitTimeMultiApproval is set', () => { + const cli = new TestCLI(); + + const expectedLogs = { + ok: + [ [ 'Approvals: 4' ], + [ '- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624' ], + [ '- Quux User (@Quux): LGTM' ], + [ '- Baz User (@Baz): https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236' ], + [ '- Bar User (@bar) (TSC): lgtm' ] ], + info: [ [ 'This PR was created on Fri, 30 Nov 2018 17:50:44 GMT' ] ] + }; + + const youngPR = Object.assign({}, firstTimerPR, { + createdAt: LT_48H + }); + + const data = { + pr: youngPR, + reviewers: allGreenReviewers, + comments: commentsWithLGTM, + reviews: approvingReviews, + commits: simpleCommits, + collaborators, + authorIsNew: () => true, + getThread() { + return PRData.prototype.getThread.call(this); + } + }; + const checker = new PRChecker(cli, data, { waitTimeMultiApproval: 23 }); + + const status = checker.checkReviewsAndWait(new Date(NOW)); + assert(status); + cli.assertCalledWith(expectedLogs); + }); + it('should error when PR is younger than 48h and older than 47h', () => { const cli = new TestCLI(); @@ -326,6 +363,73 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should succeed with 1 approval with waitTimeSingleApproval set', () => { + 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' ] ] + }; + + const youngPR = Object.assign({}, firstTimerPR, { + createdAt: LT_48H + }); + + const data = { + pr: youngPR, + reviewers: singleGreenReviewer, + comments: commentsWithLGTM, + reviews: approvingReviews, + commits: simpleCommits, + collaborators, + authorIsNew: () => true, + getThread() { + return PRData.prototype.getThread.call(this); + } + }; + const checker = new PRChecker(cli, data, { waitTimeSingleApproval: 0 }); + + const status = checker.checkReviewsAndWait(new Date(NOW)); + assert(status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error with 0 approval and waitTimeSingleApproval=0', () => { + const cli = new TestCLI(); + + const expectedLogs = { + info: + [ [ 'This PR was created on Fri, 30 Nov 2018 17:50:44 GMT' ] ], + error: + [ [ 'Approvals: 0' ] ] + }; + + const youngPR = Object.assign({}, firstTimerPR, { + createdAt: LT_48H + }); + + const data = { + pr: youngPR, + reviewers: noReviewers, + comments: commentsWithLGTM, + reviews: approvingReviews, + commits: simpleCommits, + collaborators, + authorIsNew: () => true, + getThread() { + return PRData.prototype.getThread.call(this); + } + }; + const checker = new PRChecker(cli, data, { waitTimeSingleApproval: 0 }); + + const status = checker.checkReviewsAndWait(new Date(NOW)); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + it('should log as expected if PR can be fast-tracked', () => { const cli = new TestCLI();