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

meta: add policy to land PRs without approval #336

Closed
wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

As discussed in the 2020-02-12 Diagnostics WG Meeting and suggested in
#242 (comment),
change the landing policy so that Collaborators can land pull requests
if it was open for three days and there were no reviews on it.

Ref: nodejs/diagnostics#355

As discussed in the 2020-02-12 Diagnostics WG Meeting and suggested in
nodejs#242 (comment),
change the landing policy so that Collaborators can land pull requests
if it was open for three days and there were no reviews on it.

Ref: nodejs/diagnostics#355
@mmarchini
Copy link
Contributor Author

cc @nodejs/diagnostics @nodejs/llnode @nodejs/post-mortem please let me know if there are any objections

@@ -9,3 +9,6 @@ with the following differences:
- There's no minimum wait time for landing a pull request. Pull requests can
land as soon as they get approved by at least one Collaborator.
- Approval must be from Collaborators who are not authors of the change.
- If the pull request was open for three days (72 hours) without reviews, and
Copy link
Contributor

Choose a reason for hiding this comment

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

Not to intentionally slow llnode down any further, but what about 7 days to match the one approval clause from https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#code-reviews.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely opposed to 7 days, but I don't think "to match the one approval clause from core" is a good reason to do so. I'm open to hearing other arguments to use 7 days instead of 3 though.

On a counter-argument to 7 days, if there is a bug and a PR with a fix doesn't get a review, it will now take seven days to release a fix. We could have a shorter waiting period for bug fixes, but that seems to overly complicate the policy.

Copy link
Contributor

Choose a reason for hiding this comment

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

To counter your counter-argument, if there is a bug under this proposal, it will take 3 days to release a fix 😄. I guess any amount of waiting will be an arbitrary amount. I only recommended going with 7 days because that was the agreed upon answer to this exact same problem in core. At the end of the day, you're the one doing literally all of the work, and I'm not going to block this PR.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mmarchini
Copy link
Contributor Author

Keeping this open until tomorrow afternoon (PST) in case there are any objections from folks who didn't comment yet

mmarchini added a commit that referenced this pull request Feb 22, 2020
As discussed in the 2020-02-12 Diagnostics WG Meeting and suggested in
#242 (comment),
change the landing policy so that Collaborators can land pull requests
if it was open for three days and there were no reviews on it.

Ref: nodejs/diagnostics#355

PR-URL: #336
Refs: nodejs/diagnostics#355
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mmarchini
Copy link
Contributor Author

Landed in bd3a1c7

@mmarchini mmarchini closed this Feb 22, 2020
@coveralls
Copy link

coveralls commented Aug 24, 2024

Pull Request Test Coverage Report for Build 810fbce37482eb1949ea07dcbc019ce9a32ff0a8-PR-336

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 212 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+4.5%) to 78.618%

Files with Coverage Reduction New Missed Lines %
src/constants.cc 1 80.3%
src/error.h 1 87.5%
src/llv8.cc 3 71.04%
test/common.js 7 80.08%
src/llnode_api.cc 8 87.5%
src/node-inl.h 11 0.0%
src/node.h 12 20.0%
test/plugin/workqueue-test.js 13 55.17%
src/node.cc 21 37.5%
src/node-constants.cc 38 31.88%
Totals Coverage Status
Change from base Build bff9f5ea3e75607c7828bab04f87c2dfc03a5dcd: 4.5%
Covered Lines: 3709
Relevant Lines: 4705

💛 - Coveralls

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

Successfully merging this pull request may close these issues.

6 participants