Closed mmarchini closed 4 years ago
cc @nodejs/llnode and @nodejs/diagnostics
If this turns out to be a bad idea we can always go back to the previous policy later.
Merging #296 into master will not change coverage. The diff coverage is
n/a
.
@@ 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.
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.
I'm open to 1-apporval-whenever if others are.
Updated COLLABORATORS_GUIDE with 1-apporval-whenever. Please let me know what y'all think :)
Landed in afaec48
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.