feat: check large pull request approvals in git node land - #1173
feat: check large pull request approvals in git node land#1173zeexzeex wants to merge 2 commits into
Conversation
2149ab7 to
f8afeef
Compare
|
I don't think that trying to automatically detect whether a PR falls into the policy definition of a large PR is ever going to be feasible. This can't detect whether a PR adds a new subsystem, or detect whether a non-automated dependency change PR meets the large PR threshold (it's only automated commits that are exempt), and indeed a PR only needs to touch a single gypfile in I think we would be much better off making this a label-based check, à la |
|
You're right. I checked against the I'll rewrite this as a |
Large pull requests follow the same approval path as semver-major changes: at least two TSC approvals. `git node land` did not check for this, so such a pull request could land with fewer. Whether a pull request is large is not something that can be derived from the diff. The policy counts a new subsystem as large regardless of size, and exempts routine dependency updates but not other dependency changes, neither of which is visible in the pull request data. The project already records the judgement with the `large-pr` label, so check that, the way `semver-major` is checked. Refs: https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md Signed-off-by: Avocado <ujubongbong@gmail.com>
f8afeef to
169b769
Compare
43f08d1 to
d456189
Compare
Keep the existing semver-major approval path intact and check large-pr labels in their own branch.
d456189 to
fff3b82
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1173 +/- ##
==========================================
+ Coverage 57.81% 57.91% +0.09%
==========================================
Files 53 53
Lines 10398 10421 +23
==========================================
+ Hits 6012 6035 +23
Misses 4386 4386 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| 'large pull requests require at least 2 TSC approvals'; | ||
| cli.error(message); | ||
| this.addReason( | ||
| PR_CHECK_REASON_CODES.MISSING_LARGE_PR_TSC_APPROVAL, |
There was a problem hiding this comment.
We can use the existing code here, rather than create a new one.
Node.js requires two TSC approvals for large pull requests, but nothing enforces it at land time. This adds that check to
git node land.Whether a pull request is large is not something that can be derived from the diff. The policy counts a new subsystem as large regardless of size, and exempts routine dependency updates but not other dependency changes; neither is visible in the pull request data. An earlier revision of this PR tried a 5000 line threshold, and checking it against the 6 pull requests currently carrying the
large-prlabel, it missed 2 of them: nodejs/node#64429 is well under the threshold, and nodejs/node#62217 has a net negative diff.The project already records this judgement with the
large-prlabel, so this checks that instead, the waysemver-majoris checked. The approval requirement is the same for both, so the existing branch is widened rather than duplicated, with a distinct message and reason code so a failure tells the author which rule applies. A pull request carrying both labels is reported as semver-major.Refs: https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md
Fixes: #1063