-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Proposal: remove minimal time limit for semver-patch prs #33627
Comments
The minimum time is actually 48 hours. I suggest to open a PR to change/remove the minimal waiting time? |
Yeah, I'll open a PR soon but I want to see if there were any immediate objections first |
@jasnell I definitely recognize that it can be a needless delay for a lot of folks depending on the substance of a given PR, but i do think there's value in some minimum at least for time zone purposes. I'd be concerned that a potential stakeholder to a PR would miss out on the opportunity to review with no minimum at all if we don't allow them at least some reasonable time to catch it. What would you think about dropping down to 24 hours? I think that, while short enough to cause minimal imposition, at least casts a net across timezones. |
I think it's better to keep it as it is because:
I think the fast tracking mechanism is already good enough for cases where it's absolutely necessary. Additionally, I think by allowing more time for review it might help prevent the need for "fast tracked" followup PRs to fix missed issues with recently pushed PRs for example. |
@codebytere ... The delay just seems... Pointless ... Most of the time. In practice, I'd argue that most trivial prs would stay open for at least 24 hours anyway while any potential issues are worked through, or we know they're trivial and receive a bunch of people signing off quickly without any fanfare. Requiring the additional process just ends up feeling like process for the sake of it. If changes end up landing that shouldn't have, quick reverts are easy. |
I regularly catch bugs, sometimes pretty severe bugs, in 10 line PRs with several sign-offs so yeah, I think there's value in not rushing things through. I like my merging like I like my dancing: slow, as to avoid accidents. :-) |
Alternatively, the first author approving a "trivial" change can suggest fast-tracking, and as soon as another collaborator approves the PR + fast tracking (and CI is green etc.), it can land. |
Given we sent out a contributor survey recently (#33583) I'd be interested if the minimal wait time came up in the responses. |
@richardlau ... the short answer is yes... the longer answer, which I will explore in the survey report back to the TSC is that a good number of survey respondents indicate frustration with our current contribution process. |
The results of the contributor survey (in nodejs/TSC#882) make it sound like this is definitely a problem. I would also prefer to merge PRs faster, but I also feel like that should come with a more relaxed reverting policy; e.g., merge PRs without waiting, but also revert them without waiting¹, and if a collaborator requests that a PR gets a 48-hour waiting period for any reason, we respect that. I also think this would reduce the amount of nits and optimization requests on PRs – because, honestly, it’s usually easier to perform minor fixups by oneself instead of commenting nits on PRs. It’s just hard to do that in our current model. ¹ (and without a separate PR |
Opened #33879 which would implement that suggestion – I’m not set on that particular solution, but I’d like to see something happen. |
Here is why I am a bit skeptical: a PR on http or streams typically needs some scrutiny from @nodejs/http or @nodejs/streams to not regress in performance or correctness. There have been quite a few cases where PR got a lot of “quick” LGTM from people outside said teams and a comment from one of those members that “changed everything”. This being said, after it gets a few reviews from said teams, there is little to gain with waiting more time. I think other maintainers feel the same on some of the subsystems they maintain. I think we should take a look at CODEOWNERS, as I think it would simplify things quite a bit. Overall I’m happy to try dropping the time as long as reverts become easier than they are today. In case something should not have landed the policy should be “revert immediately and ask questions later”. |
@mcollina That’s 💯 what I was going for in #33879 – I’m happy to adjust the proposal if you think it could be performed in a better way. |
Revisiting codeowners for specific subsystems would be a good idea in general, separate from this... Especially given the feedback in the survey that it's often difficult to find the right people to discuss a given change |
+1 for revisiting codeowners. I'm not opposed to changes to our process but the current process is a balance between having some checks in place for things such as streams that historically have been shown to need reviews and trivial one-word doc changes at the other end of the scale. Part of that balance is to have a simple contribution process that can be explained to someone not familiar with our codebase. Once we get into differing processes depending on what has changed it becomes harder to determine when something can land. Automation can help there, but our current automation hasn't kept up with out existing process (for example, |
Closing this as we are moving forward with the codeowners experimental and @addaleax has a PR open to address the time limit change. |
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: - Drops the 48-hour/7-day waiting times as strict rules. - Drops the `fast-track` label, which is now the implied default. - Adds a `wait-for-feedback` label that collaborators can add if they think that a pull request *should* remain open for at least 48 hours. - Reduces the strict requirement for merging something to 1 approval, and keep 2 approvals as a strong suggestion. - Allows immediate reverting of pull requests that have been fast-tracked, if deemed necessary. [2020 Node.js Contributors Survey]: nodejs/TSC#882 Refs: nodejs/TSC#882 Fixes: nodejs#33627
@nodejs/tsc ... As we all painfully are aware, we enforce a minimal time period of
7248 hours to land all prs unless those are fast tracked. This can be rather.... Irritating.... When dealing with small patch changes and most of the time just causes needless delay.I would like to propose that we completely eliminate the minimum time requirement for all non-semver minor and major prs. Those would still require minimal sign off from at least two contributors and passing ci with no outstanding objections.
If a pr lands that retroactively is recognized as being minor or major, then we would just revert.
The text was updated successfully, but these errors were encountered: