fix: treat fast-track with not enough approvals as non-fatal

[LiviaMedeiros]

If a PR has fast-track with not enough approvals, instead of exiting with error we can proceed to see if it meets regular time requirements.
This also makes error more informative, showing these requirements.

[anonrig]

Can you update the tests?

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (ebcf18e) 83.41% compared to head (14a94b6) 83.43%.

Additional details and impacted files

@@ Coverage Diff @@## main #676 +/- ##
==========================================
+ Coverage 83.41% 83.43% +0.02% 
==========================================
Files 37 37 Lines 4136 4142 +6 ==========================================
+ Hits 3450 3456 +6 
Misses 686 686 

Impacted Files	Coverage Δ
lib/pr_checker.js	98.31% <100.00%> (+0.01%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[aduh95]

Can we add a more precise error message when there's not enough approval, similar to what was done in

node-core-utils/lib/pr_checker.js

Lines 225 to 227 in 3cc326a

	timeLeftMulti=timeLeftMulti<0||isFastTracked ? 0 : timeLeftMulti;
	cli.error(`This PR needs to wait ${timeLeftSingle} more hours to land `+
	`(or ${timeLeftMulti} hours if there is one more approval)`);

Something like This PR needs to wait ${timeLeftSingle} more hours to land (or 0 minutes if there is ${missingFastTrackApprovals} approval of the fast-track request from collaborators).

[aduh95]

Maybe we could have the CQ remove the https://github.com/nodejs/node/labels/fast-track label on PRs that have been opened for more than 48h, so we can make statistics on how many PRs were actually fast-tracked (but AFAIK no one is doing any statistics, so it wouldn't be a big deal if we would simply ignore it).

[targos]

Maybe we could have the CQ remove the https://github.com/nodejs/node/labels/fast-track label on PRs that have been opened for more than 48h, so we can make statistics on how many PRs were actually fast-tracked (but AFAIK no one is doing any statistics, so it wouldn't be a big deal if we would simply ignore it).

Why not, if it's not too difficult. I'd go even further with this fix and not warn at all. After 48 hours, the fast-track request isn't relevant anymore.

[LiviaMedeiros]

Can we add a more precise error message when there's not enough approval, similar to what was done in

The combination of warning for fast-track and error for time was for the same thing. Does updated message look better?

After 48 hours, the fast-track request isn't relevant anymore.

AFAICT it's not relevant for PRs with 2 or more approvals, but still might make sense for 7 days if there's only one?

I think, ideally CQ should "wait" for fast-tracked PRs to have enough approves rather than exiting with error.

[aduh95]

I think, ideally CQ should "wait" for fast-tracked PRs to have enough approves rather than exiting with error.

That's not really practical, as the CQ can't discriminate with a simple request a ready fast-tracked PR from one who lacks approvals. So if we don't remove the commit-queue label, it forces the CQ to go to the trouble of cloning the repo etc. every 5 minutes. I think the current way of doing things is a good compromise.

After 48 hours, the fast-track request isn't relevant anymore.

AFAICT it's not relevant for PRs with 2 or more approvals, but still might make sense for 7 days if there's only one?

No, as fast-tracked PR needs at least two approving reviews to land – and once it has two, it doesn't need to be fast-tracked anymore. (Refs: https://github.com/nodejs/node/blob/6831e2fb8814b3c1d7430471fc08dcb8543d4509/doc/contributing/collaborator-guide.md#L194-L196)

[aduh95]

I think we need to fix the grammar here, even if it makes the code uglier.

[aduh95]

Could we add a test where there is only one approving review, and the fast-track request has only one or 0 👍? Non-blocking question, this PR can also land without it.