Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: require two approvals to land changes #22255

Closed
wants to merge 6 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 10, 2018

First in a series that will hopefully streamline/simplify our policies. I don't want this to slip in unnoticed and it's a significant procedural change being proposed, so: @nodejs/collaborators

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.

Refs: #19564

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. labels Aug 10, 2018
@ChALkeR
Copy link
Member

ChALkeR commented Aug 10, 2018

LGTM, though I am curious how many (and which exact) changes landed with only one collaborator review this year. That might give us some hints how would this affect the review time and what changes might become harder to land. I expect close to 0.

@jasnell
Copy link
Member

jasnell commented Aug 10, 2018

I'm generally -1 on this as I don't see the change as being necessary. 1 approval should be enough if the change is relatively constrained and the list of people who have the necessary domain knowledge to adequately review is small.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it explicit

@Trott
Copy link
Member Author

Trott commented Aug 10, 2018

@ChALkeR Here are all the ones since the start of July.

d3d54aa
284caaa
3095eec
87f7671
f7454a5
a706456
ededb4b
57e3015
85b0f16
f386c7e
e021ca2

That's 11 commits. For perspective, there were 352 commits in that time, so were talking about approximately 3.1% of commits.

Copy link
Member

@bengl bengl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With well over 100 collaborators, this seems reasonable.

sufficient expertise who is able to take full responsibility for the
change. In the case of pull requests proposed by an existing
Collaborator, an additional Collaborator is required for sign-off.
All pull requests must be reviewed and accepted by two Collaborators with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a good opportunity to clarify that we mean at least two, and not exactly two.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering those numbers and the types of commits, I do not see this as needed.

@joyeecheung
Copy link
Member

Maybe we can make exceptions for certain subsystems? AFAICT crypto may need that.

@Trott
Copy link
Member Author

Trott commented Aug 10, 2018

1 approval should be enough if the change is relatively constrained and the list of people who have the necessary domain knowledge to adequately review is small.

@jasnell The last time we reviewed changes that had only one approval, none of them fit that description. (Specifically: None of them were "There's nobody who can review this code because it requires specific domain knowledge.") I think preserving the one-approval rule for that reason is preserving it for a problem that we don't actually have. As I wrote at that time:

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.

@Trott
Copy link
Member Author

Trott commented Aug 10, 2018

Considering those numbers and the types of commits, I do not see this as needed.

@mcollina With only one exception (85b0f16), all of the commits seem like they could have obtained more reviews with just a little effort. I mean, look at f386c7e.

Time to repeat the exercise from last time:

  • d3d54aa is a straightforward Makefile change that could have gotten a second review. Certainly the issue here isn't that there aren't people with sufficient expertise to review it. Indeed, I'd be surprised if that PR was unable to get four or five additional reviews.

  • a706456, a706456 and 284caaa are actually all the same commit. It's a V8 backport from upstream that got re-applied when we updated V8. Anyway, we routinely get multiple reviews on V8 backports and there is little doubt that additional reviews could have been amassed for that were it required. The relevant PR was also open for less than the required 72 hours on a weekend, so that may be the reason for only one review.

  • 87f7671 and f386c7e are simple doc changes. It would have been simple to get more reviews for them.

  • 3095eec is not complex and another review would have certainly been attained had it been sought. No groups were @-mentioned and @ChALkeR even chimed in with an after-the-fact LGTM.

  • f7454a5 is the one where things may have been delayed. However, that one doesn't exactly make the case because it was landed in violation of the 48/72 hour rule. (It was open for less than 72 hours on a weekend.) It's entirely possible (likely?) that it would have obtained a second review had it been open long enough.

  • With ededb4b and 57e3015, there were no attempts to @-mention any relevant teams. I'd argue that indicates insufficient effort to get multiple reviews (which is OK in the current setup since that's all that's required--but doesn't exactly make it a foregone conclusion that additional reviews couldn't be obtained if they were sought).

  • 85b0f16 is a case where @tniessen tried repeatedly to get the attention of @nodejs/crypto but only got one review. So that's one commit that might have run into trouble with this process.

  • e021ca2 has no team ping. Seems like something that would have gotten more reviews if they were sought.

@Trott
Copy link
Member Author

Trott commented Aug 10, 2018

Maybe we can make exceptions for certain subsystems? AFAICT crypto may need that.

@joyeecheung Maybe, although that undermines the simplicity/consistency aspect. The only recent commit where reviews were actively sought and not obtained was a crypto one (85b0f16). On the other hand, that is the only crypto PR that has landed this year with only one review. And there have been a good number of crypto commits. So maybe it's not such a problem for crypto after all? Kind of interested in what @tniessen thinks, as he's the one that was trying to get reviews and only got one.

@jasnell
Copy link
Member

jasnell commented Aug 10, 2018

@Trott... for me, there's no exercise to go through... this appears to be solving for a problem that doesn't exist.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

@refack
Copy link
Contributor

refack commented Aug 10, 2018

IMHO velocity on it's own is not a goal. Having more review should be a great tool to improve code quality, which should be of higher priority.

I'd also like for us to put more emphasis on the other parts of that paragraph:

All pull requests must be reviewed and accepted by a Collaborator with sufficient expertise who is able to take full responsibility for the change.

It seems to me that this will go to a vote. In that case we might want to consider expanding the scope of this change (longer review window, cap maximum of approvals, require /CC of "code owners")

Ref: nodejs/CTC#144

@addaleax
Copy link
Member

addaleax commented Aug 10, 2018

Having more review should be a great tool to improve code quality, which should be of higher priority.

This. 💯 If we can’t get enough reviewers, then we really should force ourselves to put effort into making more people familiar with the relevant code.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I agree with @joyeecheung tough that an exception could/should be made for crypto.

@jasnell
Copy link
Member

jasnell commented Aug 11, 2018

Having more reviewers per PR is a fantastic goal. Imposing additional process does not accomplish that, encouraging more folks to get involved does.

@Trott
Copy link
Member Author

Trott commented Aug 11, 2018

Having more reviewers per PR is a fantastic goal. Imposing additional process does not accomplish that

  • This does not impose additional process. Increases existing requirements from one review to two reviews? Yes. Imposes additional process? No. It clarifies and minimally increases requirements to what is in fact the de facto for approximately 97% of our commits.

  • This rule will absolutely increase the number of reviewers for PRs. It is impossible IMO to look at doc: fix HTTP res 'finish' description #21670 and reasonably believe that this rule wouldn't have increased the number of reviews that PR got at least from one to two. You could argue that it is not worthwhile to get more reviews on such a PR--I would dispute that, but one can reasonably argue it--but you'd have to embrace some pretty unlikely scenarios to conclude that PR would not get a second review. Ping the documentation team and the http team and you'd have a second review in no time. (Or addaleax would have reviewed it before landing it.)

@benjamingr
Copy link
Member

One problem is that for some PRs in less glorious and well known parts of the code getting even one review is hard.

Can we get an estimate of how many pull requests did not get a single review in 3 days?

@benjamingr
Copy link
Member

Another issue is that people might not be as responsible when reviewing (e.g run the actual code and play with the changes) when reviewing if they think at least one other person is responsible. I think this has a name in psychology.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m going to +1 since I think more eyes is good - but I eventually would like us to work towards a better review process.

jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23249
Refs: #22255
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 22, 2018
Refs: nodejs#22255

PR-URL: nodejs#24561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
targos pushed a commit that referenced this pull request Nov 24, 2018
Refs: #22255

PR-URL: #24561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Refs: #22255

PR-URL: #24561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
joyeecheung pushed a commit to nodejs/node-core-utils that referenced this pull request Dec 3, 2018
One Collaborator approval is enough only if the pull request
has been open for more than 7 days

Refs: nodejs/node#22255
codebytere pushed a commit that referenced this pull request Jan 13, 2019
Refs: #22255

PR-URL: #24561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Refs: nodejs#22255

PR-URL: nodejs#24561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
codebytere pushed a commit that referenced this pull request Jan 29, 2019
Refs: #22255

PR-URL: #24561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
codebytere pushed a commit that referenced this pull request Jan 29, 2019
Refs: #22255

PR-URL: #24561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@Trott Trott deleted the simplify-approval branch January 13, 2022 22:50
johnfrench3 pushed a commit to johnfrench3/core-utils-node that referenced this pull request Nov 2, 2022
One Collaborator approval is enough only if the pull request
has been open for more than 7 days

Refs: nodejs/node#22255
renawolford6 added a commit to renawolford6/node-dev-build-core-utils that referenced this pull request Nov 10, 2022
One Collaborator approval is enough only if the pull request
has been open for more than 7 days

Refs: nodejs/node#22255
Developerarif2 pushed a commit to Developerarif2/node-core-utils that referenced this pull request Jan 27, 2023
One Collaborator approval is enough only if the pull request
has been open for more than 7 days

Refs: nodejs/node#22255
gerkai added a commit to gerkai/node-core-utils-project-build that referenced this pull request Jan 27, 2023
One Collaborator approval is enough only if the pull request
has been open for more than 7 days

Refs: nodejs/node#22255
shovon58 pushed a commit to shovon58/node-core-utils that referenced this pull request Jun 9, 2023
One Collaborator approval is enough only if the pull request
has been open for more than 7 days

Refs: nodejs/node#22255
patrickm68 added a commit to patrickm68/NodeJS-core-utils that referenced this pull request Sep 14, 2023
One Collaborator approval is enough only if the pull request
has been open for more than 7 days

Refs: nodejs/node#22255
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.