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

Reaching a decision on "drop minimum wait time" #921

Closed
mmarchini opened this issue Sep 10, 2020 · 5 comments
Closed

Reaching a decision on "drop minimum wait time" #921

mmarchini opened this issue Sep 10, 2020 · 5 comments

Comments

@mmarchini
Copy link
Contributor

nodejs/node#33879 has been blocked for a while, some suggestions to mitigate the concerns were raised but the objector hasn't commented on those suggestions. Therefore I would like for the TSC to step in the PR and decide if it should be closed, if it should move forward as is, or if it should move forward with modifications (and then list what those modifications should be).


PR Summary

PR reason

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.

Proposed change

  • 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.

Objection

Compromise suggestions

nodejs/node#33879 (comment), nodejs/node#33879 (comment):

  • If a particular piece of code has one or more CODEOWNERS and one or more of those have signed off and the PR has passed CI, skip the 48 hour rule
  • Our current rules say that a PR that does not have at least two sign offs must wait a full 7 days before it can land. Reduce that to 3 days (that is, so long as the PR has at least one sign off and passes CI, it can land after 3 days).

nodejs/node#33879 (comment):

  • Drop the revert rule to avoid "internet drama". Reverts would follow the same rules as normal PRs

cc @nodejs/tsc

@mhdawson
Copy link
Member

I'm +-0 with my main concern being the "immediate revert" but not to the point where I'd block (and have not in the issue). I have some worries but am willing to see how the changes work out assuming the rest of the TSC feels its the right way to go.

@mcollina
Copy link
Member

If a particular piece of code has one or more CODEOWNERS and one or more of those have signed off and the PR has passed CI, skip the 48 hour rule

I think this is the best solution.

@MylesBorins
Copy link
Contributor

+1 to this plan

@mhdawson
Copy link
Member

I'm +1 to what @mcollina suggested:

If a particular piece of code has one or more CODEOWNERS and one or more of those have signed off and the PR has passed CI, skip the 48 hour rule

@mhdawson
Copy link
Member

Believe this has been addressed, closing. @mmarchini let me know if you think that was not the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants