Great thread! RE: > *I assume we ultimately need a committer to review the work of > anon-committer ahead of merge ( since someone with permissions > eventuallyhas to actually accept/merge/commit the code ). In general, is > onereviewer/committer sufficient?* > IMO, yes, one reviewer who is also a committer is sufficient > > *How does this change if the author of the PR is an existing committer?* > If I am a committer and I make a PR, I would wait for another committer to > review and approve. However, if it's been a week and no one approves, I > would send a review request to dev@, and if there is no response after a > couple of weeks, then I'd go ahead and merge it myself (given all > tests/checks have passed of course) > > Thanks for the discussion! > - Gedd
I like Gedd’s pragmatic take on this. All PR’s *should* get a review, whether from a committer or contributor. I will add that it probably should be PPMC that reviews (not all committers are PPMC), until we have some set of designated release managers that may or may not be PPMC. I agree with Rob that one review should be sufficient. There are some alt-cases that merit discussion, such as: 1. whether we need to review a PR if it comes from a committer, and it incorporates only minor changes to update package dependencies (basic maintenance)? Should all basic maintenance be handled via PR? That seems extreme to me... 2. how to handle merges during release procs by a release manager? Right before a VOTE, there are always a bunch of tiny things—DRAT/Licensing, README corrections, URLs, etc. Should those be handled by PR? I think a Test branch takes care of a lot of this: new PRs and merges made against a test branch, then maybe independent PPMC reviews and merges with Master? Great thread! Thanks for starting Austin. Sorry I’m late to the party! Josh
