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 |
f8afeef to
169b769
Compare
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 58.13% 58.23% +0.09%
==========================================
Files 53 53
Lines 10406 10429 +23
==========================================
+ Hits 6050 6073 +23
Misses 4356 4356 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fff3b82 to
8be170a
Compare
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>
Keep the existing semver-major approval path intact and check large-pr labels in their own branch, sharing the TSC approval lookup between them. Signed-off-by: Avocado <ujubongbong@gmail.com>
8be170a to
6e3ad3b
Compare
2 participants
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