On 09/08/2014 02:52 PM, Russell Bryant wrote: > On 09/08/2014 05:17 AM, Steven Hardy wrote: >> On Mon, Sep 08, 2014 at 03:14:24PM +1200, Robert Collins wrote: >>> I hope the subject got your attention :). >>> >>> This might be a side effect of my having too many cosmic rays, but its >>> been percolating for a bit. >>> >>> tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead >>> use 'needs 1x+2'. We can ease up a large chunk of pressure on our >>> review bottleneck, with the only significant negative being that core >>> reviewers may see less of the code going into the system - but they >>> can always read more to stay in shape if thats an issue :) >> >> I think this may be a sensible move, but only if it's used primarily to >> land the less complex/risky patches more quickly. >> >> As has been mentioned already by Angus, +1 can (and IMO should) be used for >> any less trival and/or risky patches, as the more-eyeballs thing is really >> important for big or complex patches (we are all fallible, and -core folks >> quite regularly either disagree, spot different types of issue, or just >> have better familiarity with some parts of the codebase than others). >> >> FWIW, every single week in the Heat queue, disagreements between -core >> reviewers result in issues getting fixed before merge, which would result >> in more bugs if the 1x+2 scheme was used unconditionally. I'm sure other >> projects are the same, but I guess this risk can be mitigated with reviewer >> +1 discretion. > > Agreed with this. I think this is a worthwhile move for simpler > patches. I've already done it plenty of times for a very small category > of things (like translations updates). It would be worth having someone > write up a proposal that reflects this, with some examples that > demonstrate patches that really need the second review vs others that > don't. In the end, it has to be based on trust in a -core team member > judgement call. >
What about simply leaving it as-is and allow people to ninja-approve things when they feel like it? When in doubt, reviewers should just stick to the 2x+2 and don't approve the patch. I'm basically saying the same thing that has been proposed but based on the current default + an exception to the rule. What changes is the way people approach reviews. Leaving 2x+2 as the default but allowing exceptions where people can 1x+2A is better than making the default 1x+2 and asking people to not approve patches if they think it requires more reviews. Just my $0.02, I agree with the proposal of trusting cores and letting them do 1x+2A when they think it's worth it. Flavio -- @flaper87 Flavio Percoco _______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
