X-posting to public-infra for those that have missed the conversation going on in WebApps on the subject of test review within the web-platform-tests repository on GitHub.
On Tuesday, April 23, 2013 at 9:55 AM, James Graham wrote: > On 04/23/2013 08:43 AM, Robin Berjon wrote: > On 22/04/2013 13:12 , James Graham wrote:On Mon, 22 Apr 2013, Arthur Barstow > wrote:The only thing that we ask is that pull requests not be merged by > > > > > whoever made the request. > > > > > > > > > > > Is this to prevent the `fox guarding the chicken coop`, so to speak? > > > > > If a test facilitator submits tests (i.e. makes a PR) and everyonethat > reviews them says they are OK, it seems like the facilitator > > > > should be able to do the merge. > > > > > > > Yes, my view is that Robin is trying to enforce the wrong condition > > > here. > > > > > No, I'm just operating under different assumptions. As I said before, > ifsomeone wants to review without having push/merge powers, it's > perfectlyokay. I don't even think we need a convention for it (at this > point). Ido however consider that this is an open project, so that > whoeverreviews tests can be granted push/merge power. > > > Why? Because the alternative is this: you get an "accepted" comment > fromsomeone on a PR. Either you trust that person, in which case she > couldhave merge powers; or you don't, in which case you have to review > thereview to check that it's okay. Either way, we're better off making > thatdecision at the capability assignment level since it only happens once > > per person. > > > FWIW I'm used to a situation in which the opposite approach is generally > taken; that is a reviewer is responsible for reviewing, but the > submitter is responsible for doing the final integration of their > changes.Here, the "final integration" consists of clicking the "merge button" > followed by clicking the "are you sure?" button. Hardly a place where you'll > catch a mistake. Generally, OS projects using the GH workflow tend to leave integration to the reviewers, as they're the only ones able to have access to merging content to the canonical repo. I suggest we do the same and grant collaborator access to anyone who has had one contribution merged in the canonical repo OR is a WG member. This is based on the "Pull Request Hack" system[1] described by Felix Geisendörfer. Here's a short FAQ around this proposal: 1. Who can review an merge content? Repo collaborators. 2. Who can become a repo collaborator? Anyone that fulfills either one of the below conditions: a) be a member of any WG which has tests in the web-platform-tests repo, OR b) have had at least one contribution to web-platform-tests be merged in the main repo. 3. How do you become a repo collaborator? Right now, ask Robin or myself. We're hoping to get tooling to simplify this in the near future. 4. Who is responsible for checking the CLA has been signed? The reviewer when merging a contribution (if the contributor is not a repo collaborator, no point if he/she is). We'll have tooling to help with this. --tobie --- [1]: http://felixge.de/2013/03/11/the-pull-request-hack.html
