To be clear, this is a suggestion. I’d be happy to see this PR land as-is, but I’m also happy to receive any kind of feedback, and will gladly update this PR.

Also, the wait-for-feedback label doesn’t exist yet, but this would be a prime example of a PR to which I would immediately apply it. 😉

I would like it if we at least considered what implications this could have on QA... reverting is easy... assuming that the "mistake" is noticed.

As an example, #33818, has one approval, however I find it problematic. I'm not sure I would have noticed it if it landed until possibly while going through the change log in the release proposal.

Not saying it's a problem. But it would require a change of habits, i.e. instead of just scanning open PR's labelled with modules I'm familiar with, I should also more regularly scan closed PR's and/or the commits.

Not saying it's a problem. But it would require a change of habits, i.e. instead of just scanning open PR's labelled with modules I'm familiar with, I should also more regularly scan closed PR's and/or the commits.

Yeah, that’s definitely true. I think the solution that the Chromium/V8 projects have for this is To-Be-Reviewed tags that tell specific people to review the change, but that seems hard to enforce here where we can’t really tell individuals what to do.

Also, is it actually the waiting time that is the problem or is it the fact that it takes too long to gain sufficient approval (2) to merge a PR (which can take weeks in some cases)? i.e. maybe lowering the number of required approvals to 1 is sufficient, i.e. keep the 48h waiting time.

@ronag I, personally, find that the 48h waiting time is problematic, because it forces work that builds upon other work to happen as a large number of commits in a single PR, which in turn makes review harder (and is actually counterproductive when it comes to QA, because it means that larger diffs will be reviewed at a time).

And as I said in #33627 (comment), I also think that this will reduce nitpicking and minor technical change requests a lot, because it encourages people to just go fix them on their own. I think that that’s also a big win for being able to contribute effectively.

I'm definitely +1 on eliminating the time requirement but I think we should retain the more than one signoff requirement. +1 to everything else

@jasnell How would that look like after this? Still enforcing a fixed 7-day waiting time?

I don’t want to see it become a regular occurrence that PRs are landed with only a single approval either, but I trust our collaborator base to make the right calls here, and I wouldn’t expect this to become a problem.

Based on my past experience with landing open PRs, the most common reason why PRs that only receive a single approval do so is that it is hard to find people who want to further engage with the PR. However, that also means that the extra 5 days waiting time don’t typically result in further engagement – rather, the PR just lies around until it can finally be merged. That’s not a great situation to be in.

I'd lower the period to wait for >1 signoff from 7 days to 72 hours. I've rarely ever seen the longer wait actually encourage more participation.

I'm definitely +1 on eliminating the time requirement but I think we should eliminate the signoff requirement. +1 to everything else.

but I think we should eliminate the signoff requirement

@rubys Can you clarify this a bit more? The 2-collaborator approval requirement, or the requirement for any collaborator to approve at all?

I've rarely ever seen the longer wait actually encourage more participation.

Yeah, that’s the point I’m making :) Let’s see what others think. Reducing to 72 hours seems fine to me if nobody else has a strong opinion, though.

@addaleax of the two options you mention, "the requirement for any collaborator to approve at all" comes closest, but misses the point. I do believe that commits should be reviewed. I don't believe that the merge should be gated on the review.

Options a reviewer has available: they can accept the change (with or without comment); if they see something that can be improved, they can make the improvement.; or they can cause the change to be reverted.

It is worth having a discussion as to what the process for a revert should be (when can you make it yourself, when should you request it of others), but that discussion would be moot if what I am proposing is not adopted.

In any case, the point is that reviewers still have the ability to disprove of changes.

But my experience, that is rare. More commonly, if I miss something, somebody else notices and makes the change. It is less effort for the reviewer, and I learn something by reviewing their change.

@rubys Yeah, I fully agree with your assessment of the situation.

So, with the current text in this PR, a collaborator could approve and then merge a pull request without further discussion, if they feel that that is the most reasonable action to take. I don’t believe that that is substantially different from just merging without approvals, it just adds an explicit step that amounts to saying “I looked at this and take responsibility for the merged code”.

What change would you make to that?

@bnoordhuis wrote:

The "two reviews" rule is a defense against bugs slipping through

My first open source project (over 20 years ago) was PHP. My second was Apache Tomcat. Both are ongoing concerns and have large user bases. Neither would be possible if there were quality issues. Neither required reviews before commit.

I'll make an assertion: if you increase the number of contributors, quality will improve.

The current process has the effect of dampening contributions. It certainly has affected me.

The "two reviews" rule isn't because we like red tape so much. It was instituted because code was going in unreviewed and the quality of releases suffered for it.

I'm familiar with the PHP project from around the turn of the millennium and that's not an example to emulate. PHP succeeded in spite of its core development culture, not because of it.

No opinion on Tomcat but I worked on httpd and I could tell stories. There's a reason everyone stuck with 1.3 for the longest time.

Let's keep things focused on this project shall we? It's obvious from the contributor feedback that we should do something here. I don't think we can get consensus on eliminating the sign off requirements and the time limit at the same time, at least not until we demonstrate that making the time limit change won't be a problem for QA. There has been a history of regressions in key subsystems so, for now, let's handle one change at a time.

@bnoordhuis @addaleax @jasnell Peace. I appreciate the opportunity to participate in the survey and in this discussion. If the majority opinion is to continue with contributor hostile processes, I'll elect to go emeritus.

@rubys To be clear, I am still not sure how what you have in mind differs from what this PR suggests. If you could elaborate on that, I think that would be helpful, at least for me.

At this point there is no majority opinion as we've only heard from a couple folks. The goal is definitely to improve on the developer experience but we have to balance that with other goals also.

@addaleax there is some subtle wording changes I would suggest, but that would be moot if some of the suggestions on this thread are adopted.

For example: "At least one Collaborator must approve a pull request before the pull request
lands". I'm not sure what you are trying to say here. I would consider landing a pull request a form of approval.

I'm not sure what you are trying to say here. I would consider landing a pull request a form of approval.

There are some folks who will land prs that have sufficient review without doing that review themselves and without signoff. Also, collaborators are not allowed to sign off on their own prs but are allowed to land them after they have sufficient sign off.

collaborators are not allowed to sign off on their own prs

@addaleax You were looking for an example of what I have in mind that differs from what this PR suggests. You now have an example.

@rubys Yes, that is the kind of thing I’m looking for :) It’s an interesting idea for sure, and I’ll give it some thought, as will hopefully others that weigh in here. I think Chromium/V8 also allow that, if necessary, and again, we’re not them and work very differently as a project, but if done right, I could see it being helpful (for example, if we introduce some kind of CODEOWNERS-style mechanism).

I agree with @bnoordhuis and @devsnek.

As a compromise, could we drop the waiting period to 24 hours for PRs with two reviews, and 48 or 72 hours for PRs with a single review?

Folks who object, please use the "Request Changes" button so we have a better sense of how many people are objecting these changes.

Given all the various PRs that have been opened on the topic, what is the state of CODEOWNERS? I'm good with dropping all waiting limits as long as a change to sensitive subsystems are reviewed by at least one of the respective team members.

Given all the various PRs that have been opened on the topic, what is the state of CODEOWNERS? I'm good with dropping all waiting limits as long as a change to sensitive subsystems are reviewed by at least one of the respective team members.

The bot will ping teams for awareness but nothing currently enforces a review from the teams, even for the teams that are sub teams of collaborators (#35166), e.g. #35161 has triggered code owners for http2 and net but AFAICT looks mergeable (subject to the usual minimum wait period).

Thanks @richardlau. I think certain subsystems are too delicate for short review time from somebody that is not an expert on them.

I'm with @mcollina on this. Some systems are sensitive enough that a team member should really be reviewing any changes there before landing. I'd actually propose that any subsystem with an associated team must have a team member review before landing, but eliminate wait times completely once a team member does review it. Anything without a team member would just need one general collaborator review. It's a process that can be automated to some extent too, setting rules to auto-merge PRs when they have an approval, but if there are modified files which are flagged as having a corresponding team then it'd wait for an approval from a member of that team first. 🤔

@cjihrig would you be ok with dropping the wait times entirely, but requiring codeowners teams to review the PR? That's something we can automate with ncu.

Also worth noting, if we want to add that as a requirement, we might need to push for 100% (or close to 100%) codeowners coverage in the repository.

The thing about codeowners (as I understand it at least) is that membership is completely arbitrary. For example, a few years ago we added a bunch of teams for the various subsystems so that we could ping The Right People™ for code reviews. I didn't join a single one of them, but somehow ended up in a number of them. Some people added themselves to all/most of the teams. If that same scenario can repeat for codeowners, then it doesn't give me any more confidence than the current process.

@cjihrig we can do a sweep on the teams and add some requirements to join, but I don't think that's really necessary and will just add more bureaucracy. The premise of this PR is that we trust collaborators to defer reviews they are not certain of to informed folk, and to prevent accidental merges it introduces wait-for-feedback label. Mistakes will happen, but they already happen today and I'm not convinced they will happen more frequently if we remove the wait times (and that is easy to track by looking at the number of reverts post-change). I trust our collaborators to make the right decision when deciding if a PR is ready to land or not.

@nodejs/tsc ... To resolve this we're going to need tsc to weigh in with a possible vote. Based on the conversation, I'm seeing three basic options

1. Status quo. Leave the time limits as they are
2. Remove all time limits as suggested by this pr
3. Reduce the current time limits to 24 hours with two sign offs, at least one of which is from relevant CODEOWNERS team, or 72 hours with one sign off.

If there are additional choices, please say so. I'm requesting that we schedule time during an upcoming tsc meeting to discuss and if we can't come to a resolution, put it to a vote.

I personally like number 3

Most mistakes can be reverted easily enough and we can adjust policy if we think we were too lax.

One thing we could do to split the difference for folks who feel less comfortable would be only allow the subsystem rule for codeowners that are a chartered working group (e.g. streams / build / diagnostics). This would not only allow those teams to be more autonomous, but would create more incentive to chartering.

One thing we could do to split the difference for folks who feel less comfortable would be only allow the subsystem rule for codeowners that are a chartered working group (e.g. streams / build / diagnostics). This would not only allow those teams to be more autonomous, but would create more incentive to chartering.

This makes automation harder (although not impossible). Personally I still think we should go with 2 and if problems start to happen frequently (or a big problem happens once due to us removing the time limits) we can re-add time limits. But regardless of what we choose, it must be automatable and we should automate before landing this (I can help with that).

@addaleax we discussed this in previous TSC meetings, and the decision is to go with the following compromise proposal:

• Reduce the current time limits to 24 hours with two sign offs, at least one of which is from relevant CODEOWNERS team, or 72 hours with one sign off.
• Along with a planned audit of the current members of the CODEOWNERS for each area.

Do you want to make these changes to move forward with this PR?

Teams audit will be conducted by TSC members, so you don't have to worry about that.

Do you want to make these changes to move forward with this PR?

Node.js policymaking is definitely one of the things I’m happy to not be involved in anymore :) You or anybody else can feel free to push changes here or open an alternative PR.

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

I am planning to pick this back up in the next few weeks.

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.