git-node: wait time for PR with single approval #290
Conversation
Codecov Report
@@ Coverage Diff @@
## master #290 +/- ##
==========================================
- Coverage 74.42% 74.14% -0.29%
==========================================
Files 22 22
Lines 1400 1400
==========================================
- Hits 1042 1038 -4
- Misses 358 362 +4
Continue to review full report at Codecov.
|
| @@ -132,12 +133,16 @@ class PRChecker { | |||
| */ | |||
| getWait(now) { | |||
| const createTime = new Date(this.pr.createdAt); | |||
| const timeLeft = WAIT_TIME - Math.ceil( | |||
| const hoursFromCreateTime = Math.ceil( | |||
There was a problem hiding this comment.
This seems rather complicated. I believe we can just combine checkReviews() and checkPRWait() so that the number of approvals and wait times are checked together with conditionals instead of independently, the logic would be much clearer that way.
There was a problem hiding this comment.
(The two methods are separated only because previously a human would check these two conditions separately, but now there is a relationship between the two we should do what a human brain would do here instead and combine the logic.)
|
This is how may brain currently works: if (approvals.length === 0) {
return false; // duh
}
// NOTE: a semver-major PR with fast-track should have either one of these labels removed
// because that doesn't make sense
if (labels.includes('semver-major')) {
if (tscApprovals.length >= 2) { // tsc are collaborators
// check 48 hour and return
} else {
return false; // 7 day rule doesn't matter here
}
}
// non-semver-major
if (approvals.length >= 2) {
if (fastTrack) {
return true;
}
// check 48 hour and return
} else if (approvals.length === 1) {
if (fastTrack) {
// ????
}
// check 7 day
} else {
// unreachable
}cc @Trott Is that correct? |
|
EDIT: Just realized I don't know how |
lib/pr_checker.js
Outdated
| @@ -5,6 +5,7 @@ const MINUTE = SECOND * 60; | |||
| const HOUR = MINUTE * 60; | |||
|
|
|||
| const WAIT_TIME = 48; | |||
| const WAIT_TIME_SINGLE_APPROVAL = 168; | |||
There was a problem hiding this comment.
Nit: 7 * 24 or add comment to make it obvious that this is 7 days
I'd say no for now. I think we should take advantage of the bot for that:
|
I don't think at the moment the bot has any hooks to do this after a label is added by a human? |
|
The bot receives all pull request events, which apparently include labels: |
@joyeecheung Yes, and to answer this part: if (fastTrack) {
// ????
}
// check 7 dayNo need to check |
|
Ping @trivikr do you still want to pursue this? Ideally I want to go with a big branch as described in #290 (comment) so it's easier to update later if we are ever going the change the rules again |
|
Ping :) |
|
@joyeecheung @targos I wanted to make a simple change instead of refactoring the code. |
One Collaborator approval is enough only if the pull request has been open for more than 7 days Refs: nodejs/node#22255
|
from this image:
it would be nice if |
|
@devsnek Thanks for the feedback, I merged the two logs into one line, regarding the date time improvements that should probably be done in a follow-on PR |
|
Thanks @joyeecheung for picking up the refactor task! |
One Collaborator approval is enough only if the pull request
has been open for more than 7 days
Refs: nodejs/node#22255