Skip to content
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
28 changes: 28 additions & 0 deletions docs/git-node.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -134,6 +136,14 @@ Options:
--help Show help [boolean]
```

<a id="git-node-land-optional-settings"></a>

### 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
Expand Down Expand Up @@ -230,6 +240,24 @@ $ git commit --amend -F msg.txt
git node metadata 167 --repo llnode --readme ../node/README.md
```

<a id="git-node-metadata-optional-settings"></a>

### 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.
Expand Down
20 changes: 17 additions & 3 deletions lib/pr_checker.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down
10 changes: 10 additions & 0 deletions lib/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class Session {
upstream: this.upstream,
branch: this.branch,
readme: this.readme,
waitTimeSingleApproval: this.waitTimeSingleApproval,
waitTimeMultiApproval: this.waitTimeMultiApproval,
prid: this.prid
};
}
Expand Down Expand Up @@ -78,6 +80,14 @@ class Session {
return this.config.readme;
}

get waitTimeSingleApproval() {
return this.config.waitTimeSingleApproval;
Copy link
Member

Choose a reason for hiding this comment

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

Umm, on a second thought, why are the defaults set in the PR checker, instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, that makes more sense. I'll move it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, this won't work because git node metadata doesn't use a Session. So git node land would get the defaults correctly, but git node metadata wouldn't.

We could create a MetadataSession, but it seems a bit too much. Also doesn't make sense since the idea of a session (to my understanding at least) is to save the state of a command to be resumed later.

Should we keep this here then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense, I forgot that the PR checker is also run by the metadata generator.

}

get waitTimeMultiApproval() {
return this.config.waitTimeMultiApproval;
}

get pullName() {
return `${this.owner}/${this.repo}/pulls/${this.prid}`;
}
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/data.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ const requestedChangesReviewers = {
approved: []
};

const noReviewers = {
requestedChanges: [],
approved: []
};

const approvingReviews = readJSON('reviews_approved.json');
const requestingChangesReviews = readJSON('reviews_requesting_changes.json');

Expand Down Expand Up @@ -87,6 +92,7 @@ module.exports = {
requestedChanges,
allGreenReviewers,
singleGreenReviewer,
noReviewers,
requestedChangesReviewers,
approvingReviews,
requestingChangesReviews,
Expand Down
104 changes: 104 additions & 0 deletions test/unit/pr_checker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const {
requestedChangesReviewers,
approvingReviews,
requestingChangesReviews,
noReviewers,
commentsWithCI,
commentsWithLiteCI,
commentsWithLGTM,
Expand Down Expand Up @@ -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();

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

Expand Down