Signing off on own requested changes #19564
Comments
mscdex
added
the
meta
|
Given the large number of collaborators that we now have (116 and growing), it might be reasonable to simply increase the number of required approvals from one to two in all cases. |
|
+1. Always two is easier to verify / remember. |
|
That's fine by me too. |
|
-0 ... I'm fine with saying should be 2+, I'd be -1 on making it a must be 2+ in all cases. |
|
Arguably, this is what the 48-hour waiting period is for. It gives others time to object. So another approach might be to require 2+ for only |
@jasnell Can you please explain why? (I'm not disagreeing with you, as I have yet to form my own opinion on this. Saying "I'm -1 on this" doesn't help the consensus-seeking conversation the way "I'm -1 on this because X, Y, and Z" would.) |
|
Sometimes PRs get "lost" amongst the other PRs, especially over 48/72-hour periods, so having a time window does not always work. It's especially a problem IMHO when there were objections in the original issue that led to the PR (and those who objected do not know the PR was made). |
|
|
Because it's unnecessary and there are often times when the change is so obscure or limited in scope that very few have enough context to sign off on it. Making it a must can needlessly delay landing important fixes that should be landed. I agree with @mscdex that no change should land if there are objections, and objections about whether the change should be made at all being discussed in the original issue and not the PR count towards that.
Oh! nice, I'd forgotten about that. I even think I've inadvertently not followed that rule on a couple of simple obscure minor edits. Good to have the reminder. |
|
I agree with @jasnell here. A great many http2, StreamBase, timers, etc. PRs have landed with a single approval because there aren't enough people available with the required expertise to review in a timely manner. |
|
that option only controls if you use the UI to merge a pr right? most PRs are landed by applying the diff and pushing from a collabs computer |
|
To be honest, I found these big red spots with crosses a bit intimidating and discouraging for new contributors. |
|
It seems this also means git pushing, if I get this right: https://help.github.com/articles/about-required-reviews-for-pull-requests/ (the example at the end). |
Yes, protected branches don't work with our workflow, because it's impossible to push a different commit from what was reviewed |
|
Just to point out, requiring two collaborator signoff still wouldn't solve the issue of objections from an issue not transferring to a related PR. I don't know if tooling could help with that particular problem, but at the very least we should be including something about it in the collaborator guide. I don't know if we should suggest pinging people in the PR who objected/raised concerns in the issue or not (that way they can reiterate their concerns in the PR). I still think we also need to have some way to make sure there is no bias when signing off on and landing a PR. Whether that's requiring at least two collaborators on all PRs (like we did originally way back when IIRC), incorporating some kind of deterrent, or something else. We already require an additional sign off for PRs from collaborators (to me, this seems kind of backwards anyway). I don't see why a PR from a non-collaborator should require only one signoff. The fast-track rules can still apply, that's fine by me, but otherwise we should have two signoffs for all other PRs. |
@Trott Just want to say that the number is not a very valid statstistic in my opinion. Out of 116 existing collaborators, about 11% have never submitted any review in nodejs/node since GitHub launched the review feature (late 2016), 36% have not reviewed any pull request using the GitHub UI in nodejs/node in the last three months, and 43% have not done so in the last month (I used nodejs/node-core-utils#186 with the query tweaked to I am OK with doing this with test and doc where there are more people doing reviews, but I share the sentiment with @apapirovski in that with some subsystems finding two reviewers to weight in, say, in a week, is pretty hard. Also, if the author is a non-collaborator, things could be even harder because they might not know who to ping for reviews and other collaborators might not remember to do that for them either. |
|
Doing it only for test and doc changes doesn't make sense to me as those are typically less controversial changes than say a change to code in lib/ or src/ for example. |
Yes and no. We require that a collaborator cannot sign off on their own PR, but such changes still only require a single sign off from someone else. The point about objections from issues not transferring to PRs is well taken, however. This has bit us a few times (including on my a few of my own PRs). Our process is biased towards Just Landing Things and Trusting Those Putting In The Effort, which is (in my opinion) the right approach but it does have the risk of legitimate concerns and objections being missed. I'm not sure if there's a policy solution for this, but it's certainly something we can strongly encourage folks to pay more attention to. |
|
One thing to perhaps reconsider is the tendency to "avoid the red X". That makes sense for nits and stuff, but I think we should embrace the red X for changes if you actually don't want them to land, especially as we embrace automation more and more. If there's concern that it will be discouraging to new contributors, some encouraging text can be written to compensate. I have to imagine having your change land and then having a bunch of people upset about it is more discouraging than being asked to make changes. |
|
Maybe we can tweak the rules to "at least two sign-offs, or one sign-off if no other reviews within n days"? I also agree that for more substantial changes from collaborators, we should embrace the red X. |
|
I don't think requiring only one sign-off after no reviews within n days will work either as one of the main problems I described earlier is that some PRs can easily get lost/buried in all of the other PRs, so people may not see it anymore or may forget about it if they haven't participated in that PR's conversation (or are otherwise not being notified). |
|
Do we have some examples where having only a single sign-off has resulted in a problem, but that we think one more might have avoided it? Just trying to understand to scope of the problem that we are trying to fix in this case. |
|
+1 |
|
@mhdawson Requiring one additional signoff is not foolproof, but it's better than nothing if we cannot find a way to at least transfer concerns/-1's from issue to PR, as it helps to somewhat ensure there is no bias when landing a PR. |
|
@mscdex I was just trying to see if there was some data quantifying the severity of the issue, that would help in making the case for changing. |
|
@mhdawson I guess "problem"/"severity" can be subjective. Did the example that prompted me to submit this issue cause backwards compatibility problems, break CI, or otherwise become similarly disruptive? No, not as far as we know. Is it a problem that complaints registered in the issue were completely ignored in the PR that resulted from that issue? IMO, yes. Is it a problem that our process allows one collaborator to publicly suggest a change affecting code executed during runtime, have a non-collaborator submit a PR implementing said suggestion, and then allow the same collaborator to be the only one to review, approve, and merge it? IMO, yes. |
|
Given the large number of active Collaborators, getting 2 for even obscure parts of the code base is not difficult. Problems with getting 2 reviews are almost never about the change being to an obscure part of the code base. It is nearly always about the change being too large to easily review in a short amount of time. But for large changes, we obviously want more than one person reviewing. I'm +1 on bumping the requirement up to 2-or-more Collaborators, but I also don't feel strongly enough to push for this if others object (which apparently they do, unless the above two paragraphs are more persuasive than I expect them to be). |
|
I'm in favour of at least two for all PRs, although maybe for quick reverts for unbreaking CI, etc, 1 is fine |
|
TL;DR: At least looking at the last 30 days, there appears to be little or no evidence for "it is too hard to get a second reviewer because this code is obscure". Instead, the evidence seems to point to a lack of effort to attract a second review when there is only one, which is understandable because it's not required by our rules. You got your one review, move on. To me, that argues for the proposed rule change. ("Too hard to get a second TSC reviewer" is another issue entirely.) Here are all the commits from the last 30 days that have only a single reviewer:
I would conclude that we're not using group @-mentions enough to draw attention to PRs. Seems to me like the proposed rule change would encourage people to surface PRs to relevant groups more often. |
I see the motivation there, but I disagree with that. We require two for fast-tracking and that should remain. Fast-tracking is exactly when you want more than one person to approve it. The fix should be so obvious and so critical that many people approve it. I agree with what @targos said in #19564 (comment): Make it two in all cases and then the rule is simple. |
I looked through that list of commits. Another somewhat common theme is cc'ing or requesting reviews from coworkers instead of the wider collaborator base. I'm not sure if it's related, but definitely a pattern I've noticed by subscribing to the issue tracker. |
|
Thanks for the detailed investigation, @Trott . The pattern there is very useful. @devsnek and I discussed about https://help.github.com/articles/about-codeowners/ the other day, maybe we can try putting teams in the CODEOWNERS files and see if this helps making sure PRs get multiple reviews? I would not mind we start requiring two sign-offs and suggesting people to at-mention teams for a start, and see how things go for a while. |
@cjihrig I also noticed that and I sometimes tend to ping handles as well, usually when I fix a TODO or some comments left in the code or when I remember someone has talked about the related code path before. When I don't have a particular person in mind I would try to ping a related team, but not always if I get a review in time, since pinging teams sometimes feel like spamming...maybe it's just me though. |
I definitely used to feel that way. Now I feel more like pinging a team is using the team for the very purpose for which it exists. |
|
+1 to add a CODEOWNERS file with teams on it and to start requiring two sign-offs. |
Currently, changes require approval by one Collaborator in most cases. However there are situations where two approvals are required. For example, breaking changes require two approvals from TSC members. And fast-tracking a request requires two approvals. Additionally, although only one approval is strictly required, in practice, we nearly always seek a second approval when there is only one. Lastly, concerns have been raised about (perhaps unintentionally) gaming the one-approval system by suggesting a change to someone else, and then approving that change when the user submits a pull request. This resolves (or at least mitigates) that concern. Fixes: nodejs#19564
Currently, changes require approval by one Collaborator in most cases. However there are situations where two approvals are required. For example, breaking changes require two approvals from TSC members. And fast-tracking a request requires two approvals. Additionally, although only one approval is strictly required, in practice, we nearly always seek a second approval when there is only one. Lastly, concerns have been raised about (perhaps unintentionally) gaming the one-approval system by suggesting a change to someone else, and then approving that change when the user submits a pull request. This resolves (or at least mitigates) that concern. Fixes: #19564 PR-URL: #22255 Refs: #19564 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: George Adams <george.adams@uk.ibm.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Currently pull requests submitted by non-collaborators only need a sign off by one collaborator in order to get merged. I think this rule should be tweaked to require at least two collaborators when the non-collaborator is making a change originally requested/suggested by an existing collaborator who signs off on the PR, to help remove any bias.
The text was updated successfully, but these errors were encountered: