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

doc: update collaborator guide to match the project size #296

Closed

Conversation

mmarchini
Copy link
Contributor

@mmarchini mmarchini commented Sep 23, 2019

This commits modifies our collaborator guide to match the current
project size. llnode is way smaller than nodejs/node, and requiring two
approvals to land a change/one approval with a seven day wait can be
quite bureaucratic. Two approvals represent 50% of our active reviewers
at the moment, which is quite a lot. Changing the requirements to one
approval with no wait time once a PR gets approved makes more sense for
this project, and it also matches other smaller projects across the
organization.

@mmarchini
Copy link
Contributor Author

cc @nodejs/llnode and @nodejs/diagnostics

@mmarchini
Copy link
Contributor Author

If this turns out to be a bad idea we can always go back to the previous policy later.

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #296 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #296   +/-   ##
=======================================
  Coverage   79.23%   79.23%           
=======================================
  Files          33       33           
  Lines        4229     4229           
=======================================
  Hits         3351     3351           
  Misses        878      878

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1a74b0...1ddc1e2. Read the comment docs.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

I'm OK with trying this if no one else objects.

@joyeecheung
Copy link
Member

joyeecheung commented Sep 24, 2019

I wonder do we even need two approvals here? As far as I know most projects in this organization don't have this kind of requirement, and mostly operate on 1-apporval-whenever. The two-approval requirement was also just introduced recently to Node.js.

@mmarchini
Copy link
Contributor Author

I'm open to 1-apporval-whenever if others are.

This commits modifies our collaborator guide to match the current
project size. llnode is way smaller than nodejs/node, and requiring two
approvals to land a change/one approval with a seven day wait can be
quite bureaucratic. Two approvals represent 50% of our active reviewers
at the moment, which is quite a lot. Changing the requirements to one
approval with no wait time once a PR gets approved makes more sense for
this project, and it also matches other smaller projects across the
organization.
@mmarchini mmarchini force-pushed the changes-in-collaborator-guide branch from 65f52b7 to 1ddc1e2 Compare September 24, 2019 19:09
@mmarchini mmarchini requested a review from cjihrig September 24, 2019 19:09
@mmarchini
Copy link
Contributor Author

Updated COLLABORATORS_GUIDE with 1-apporval-whenever. Please let me know what y'all think :)

mmarchini added a commit that referenced this pull request Sep 25, 2019
This commits modifies our collaborator guide to match the current
project size. llnode is way smaller than nodejs/node, and requiring two
approvals to land a change/one approval with a seven day wait can be
quite bureaucratic. Two approvals represent 50% of our active reviewers
at the moment, which is quite a lot. Changing the requirements to one
approval with no wait time once a PR gets approved makes more sense for
this project, and it also matches other smaller projects across the
organization.

PR-URL: #296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@mmarchini
Copy link
Contributor Author

Landed in afaec48

@mmarchini mmarchini closed this Sep 25, 2019
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.

5 participants