doc: drop minimum waiting time as hard guideline #33879
Conversation
addaleax
added
the
meta
|
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 |
|
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. |
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. |
@rubys Can you clarify this a bit more? The 2-collaborator approval requirement, or the requirement for any collaborator to approve at all? |
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? |
There was a problem hiding this comment.
Left some comments. FWIW, I think this will turn out worse than what we have now.
The "two reviews" rule is a defense against bugs slipping through. Lowering that to one (pull request) or zero (revert)... I expect that to negatively affect code quality and stability.
It sucks for contributors that the process can be time-consuming but it's great for end users - and there are many, many more of them than there are of us.
doc/guides/collaborator-guide.md
Outdated
| lands. One Collaborator approval is enough if the pull request has been open | ||
| for more than seven days. | ||
| At least one Collaborator must approve a pull request before the pull request | ||
| lands. If possible, waiting for a second approval is strongly recommended. |
There was a problem hiding this comment.
"If possible" - useless prefix, it's always possible to wait.
"Strongly recommended" - social pressure language. Most people will read it and think "I better follow this or face disapproval."
I'd just drop the second line altogether.
There was a problem hiding this comment.
I haven’t dropped the line (but I can do that if you feel strongly about it), but reduced it to “Waiting for a second approval is recommended but not mandatory”. Does that work for you?
| Before landing pull requests, allow 48 hours for input from other Collaborators. | ||
| Certain types of pull requests can be fast-tracked and may land after a shorter | ||
| delay. For example: | ||
| Before landing pull requests, allow for input from other Collaborators. |
There was a problem hiding this comment.
Collaborators that are in different timezones or away on local bank holidays... that's why the 48 hour rule exists.
This "one approval rule" is basically a 1-2 for collaborators from the same timezone.
| has already been landed, and you object to it in a way that cannot be addressed | ||
| through a new pull request which addresses your concerns, you can open a | ||
| pull request containing a revert and merge it without waiting for approvals or | ||
| CI runs. |
There was a problem hiding this comment.
I expect this revert rule will cause no end of bad blood and internet drama...
There was a problem hiding this comment.
I agree that revert without wait/approval may result in issues.
Maybe we should limit this to some scenarios or at least provide some examples when this is acceptable?
doc/guides/collaborator-guide.md
Outdated
| * Changes that fix regressions: | ||
| * Regressions that break the workflow (red CI or broken compilation). | ||
| * Regressions that happen right before a release, or reported soon after. | ||
| For pull requests that have only one approval, it is recommended to keep them |
There was a problem hiding this comment.
I would recommend adding “lacking an approval from the relevant team”.
There was a problem hiding this comment.
I’ll add that but as mentioned in the issue, I think this is something we could improve on a lot through CODEOWNERS.
|
@bnoordhuis wrote:
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 |
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. |
@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. |
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 |
|
@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
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. |
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:
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. |
mmarchini
removed
the
tsc-agenda
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. |
Trott
added
stalled
|
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. |
As the 2020 Node.js Contributors Survey has shown, the waiting
time for pull requests is a non-trivial obstacle to meaningfully
contributing to Node.js.
To reduce that friction, this PR:
fast-tracklabel, which is now the implied default.wait-for-feedbacklabel that collaborators can add if theythink that a pull request should remain open for at least 48 hours.
and keep 2 approvals as a strong suggestion.
fast-tracked, if deemed necessary.
Refs: nodejs/TSC#882
Fixes: #33627
@nodejs/collaborators @nodejs/tsc
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes