nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
236 stars 107 forks source link

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

Closed LiviaMedeiros closed 1 year ago

LiviaMedeiros commented 1 year ago

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 commented 1 year ago

Can you update the tests?

codecov[bot] commented 1 year ago

Codecov Report

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

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

Additional details and impacted files ```diff @@ 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](https://codecov.io/gh/nodejs/node-core-utils/pull/676?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [lib/pr\_checker.js](https://codecov.io/gh/nodejs/node-core-utils/pull/676?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-bGliL3ByX2NoZWNrZXIuanM=) | `98.31% <100.00%> (+0.01%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

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

aduh95 commented 1 year ago

Can we add a more precise error message when there's not enough approval, similar to what was done in https://github.com/nodejs/node-core-utils/blob/3cc326aecbb645aef7de7ce90a31dd290cd3e03d/lib/pr_checker.js#L225-L227

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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)