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: lazy consensus for PRs #27519

Closed
wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented May 1, 2019

Summery of proposal:

  • PRs can land after 48h with one approval
  • PRs with no changes requested and no objection can land after a week, even without explicit approval (is the spirit of lazy consensus)
  • Emphasize the importance of CI testing

I'm proposing this as a test trail.

I'm not sure if I'm alone in feeling that in the last few months there has been a lull in Collaborator interaction with the project, and it seems like more and more PRs have been left un-reviewed for long periods of time, as well as several significant PRs that had to wait the full week, and landed with only one review.

/CC @nodejs/collaborators

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 1, 2019
@refack refack added the meta Issues and PRs related to the general management of the project. label May 1, 2019
@cjihrig
Copy link
Contributor

cjihrig commented May 1, 2019

Strong -1 to landing PRs without review.

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.

I don’t think this is a good idea. There are significant risks for crypto and http. They will complicate life for streams.
Essentially all subsystems that are slow in reviewing changes will suffer the most.

@mmarchini
Copy link
Contributor

mmarchini commented May 1, 2019

Is there any way to gather some data on average review time for Pull Requests, how many landed within a week with only one approval, etc.?

I had the same feeling on llnode and proposed a similar change, but then I gathered some data (which is easier there, since there are way less pull requests) and most pull requests were reviewed quickly (within 48 hours), with only a couple of outliers.

Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

also -1

@richardlau
Copy link
Member

Absolutely needs some hard data to back changing. At the time of posting everything in https://github.com/nodejs/node/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+review%3Anone has been updated within the last five days which suggests they are not dormant. We’ve only just introduced the review wanted label, which at the moment has only been applied to three PRs.

@benjamingr
Copy link
Member

benjamingr commented May 2, 2019

I think the fact people are afraid to review unfamiliar code should not be overcome by introducing more unfamiliar un-reviewed code but by making more people familiar with the code.

I think we might want to consider "hot" reviews where the reviewer and the author discuss the changes or to discuss un-reviewed-for-a-long-time PRs once a week (in TSC meetings?)

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Landing code without reviews sound dangerous. I also like our new policy of requiring two reviews instead of only one, I don't see much of a reason to change that back to one.

@bnoordhuis
Copy link
Member

it seems like more and more PRs have been left un-reviewed for long periods of time

GH is being misleading here. #27246 and #27508 for example are on that list but they've had plenty reviews, multiple rounds even in the case of #27246, it's just that no one lgtm'd them yet.

There are a handful over a few days old that truly haven't been reviewed yet. That's maybe not good but it's not super terrible either given that most pull requests seem to get reviewed within a day.

@mhdawson
Copy link
Member

mhdawson commented May 9, 2019

I'm -1 as well as I believe reviews are important.

@cjihrig
Copy link
Contributor

cjihrig commented May 9, 2019

The feedback here seems to be overwhelmingly -1. I'm going to close this out.

@cjihrig cjihrig closed this May 9, 2019
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.