Open Stack

On 06/18/2014 05:24 AM, Steven Hardy wrote:
> On Wed, Jun 18, 2014 at 11:04:15AM +0200, Thierry Carrez wrote:
>> Russell Bryant wrote:
>>> On 06/17/2014 08:20 AM, Daniel P. Berrange wrote:
>>>> On Tue, Jun 17, 2014 at 01:12:45PM +0100, Matthew Booth wrote:
>>>>> On 17/06/14 12:36, Sean Dague wrote:
>>>>>> It could go in the commit message:
>>>>>>
>>>>>> TrivialFix
>>>>>>
>>>>>> Then could be queried with - 
>>>>>> https://review.openstack.org/#/q/message:TrivialFix,n,z
>>>>>>
>>>>>> If a reviewer felt it wasn't a trivial fix, they could just edit
>>>>>> the commit message inline to drop it out.
>>>>
>>>> Yes, that would be a workable idea.
>>>>
>>>>> +1. If possible I'd update the query to filter out anything with a -1.
>>>>>
>>>>> Where do we document these things? I'd be happy to propose a docs update.
>>>>
>>>> Lets see if any other nova cores dissent, but then can add it to these 2
>>>> wiki pages
>>>>
>>>>   https://wiki.openstack.org/wiki/ReviewChecklist
>>>>   https://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references
>>>
>>> Seems reasonable to me.
>>>
>>> Of course, I just hope it doesn't put reviewers in a mode of only
>>> looking for the trivial stuff and helping less with the big stuff.
>>
>> As an aside, we don't really need two core reviewers to bless a trivial
>> change: one could be considered sufficient. So a patch marked as trivial
>> which has a number of +1s could be +2/APRVed directly by a core reviewer.
>>
>> That would slightly reduce load on core reviewers, although I suspect
>> most of the time is spent on complex patches, and trivial patches do not
>> take that much time to process (or could even be seen as a nice break
>> from more complex patch reviewing).
> 
> Agreed, I think this actually would help improve velocity in many cases,
> provided there was sufficient common understanding of what consititutes a
> trivial patch.
> 
> The other situation this may make sense is when a non-trivial change has
> already been widely reviewed and approved then needs a last-minute minor
> rebase to resolve a simple merge conflict (e.g due to all these trivial
> patches suddenly landing really fast.. ;)
> 
> We discussed this in the Heat team a while back and agreed that having one
> core re-review and approve the patch was sufficient, and basically that
> folks could use their discretion in this situation.

Honestly, I think this is already culture in most projects. There is a
reason that we enforce 2 +2 in culture and not in code, as it allows for
fast approve with rationale. Previously approved with a merge conflict
typically falls into this category. Every group does this a little
differently, but at the end of the day the system does lean on us being
reasonable humans, which is typically a solid assumption.

	-Sean

-- 
Sean Dague
http://dague.net

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140618/f61a660c/attachment.pgp>