Skip to content

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

Closed
mmarchini wants to merge 1 commit into
nodejs:masterfrom
mmarchini:changes-in-collaborator-guide
Closed

doc: update collaborator guide to match the project size#296
mmarchini wants to merge 1 commit into
nodejs:masterfrom
mmarchini:changes-in-collaborator-guide

Conversation

@mmarchini

@mmarchini mmarchini commented Sep 23, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

cc @nodejs/llnode and @nodejs/diagnostics

@mmarchini

Copy link
Copy Markdown
Contributor Author

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

@codecov-io

codecov-io commented Sep 23, 2019

Copy link
Copy Markdown

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.

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

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

joyeecheung commented Sep 24, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

5 participants