git-node: wait time for PR with single approval

[trivikr]

One Collaborator approval is enough only if the pull request has been open for more than 7 days

Refs: https://github.com/nodejs/node/pull/22255

[codecov[bot]]

Codecov Report

Merging #290 into master will decrease coverage by 0.28%. The diff coverage is 93.87%.

@@            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

Impacted Files	Coverage Δ
lib/collaborators.js	100% <100%> (ø)	:arrow_up:
lib/pr_checker.js	96.11% <93.75%> (-2.23%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 93286f2...24b7cea. Read the comment docs.

[joyeecheung]

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?

[joyeecheung]

EDIT: Just realized I don't know how fast-track plays out if there is only one approval..(also, should we check the thumbs up message? I don't even know how we can identify the reply that needs emojis programmatically...

[targos]

Also, should we check the thumbs up message?

I'd say no for now. I think we should take advantage of the bot for that:

• The person who would like to fast-track adds the label
• The bot posts a well-known message to ask for thumbs up
• ncu can find that message and count the number of thumbs up by collaborators (can it?)

[joyeecheung]

The bot posts a well-known message to ask for thumbs up

I don't think at the moment the bot has any hooks to do this after a label is added by a human?

[targos]

The bot receives all pull request events, which apparently include labels:

[Trott]

cc @Trott Is that correct?

@joyeecheung Yes, and to answer this part:

    if (fastTrack) {
        // ????
    }
   // check 7 day

No need to check fastTrack there. If you remove that check, it checks that the PR is open for 7 days. If you only have one approval, you cannot fast track. So, it's 7 days or else return false.

[joyeecheung]

Ping @trivikr do you still want to pursue this? Ideally I want to go with a big branch as described in https://github.com/nodejs/node-core-utils/pull/290#issuecomment-427670704 so it's easier to update later if we are ever going the change the rules again

[targos]

Ping :)

[trivikr]

@joyeecheung @targos I wanted to make a simple change instead of refactoring the code. I'm little busy, feel free to pick it up.

[joyeecheung]

Rebased and updated the logic. @targos @Trott Can you take a look? Thanks!

[devsnek]

from this image:

it would be nice if was on one line, and the 91 hours was made like 3 days 18 hours

[joyeecheung]

@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

[trivikr]

Thanks @joyeecheung for picking up the refactor task!