Open Stack

On 02/24/2015 03:18 AM, Lana Brindley wrote:
> On 24/02/15 09:00, Nick Chase wrote:
>>
>> On 2/23/2015 1:52 PM, Andreas Jaeger wrote:
>>> Thanks for bringing it up. We do have guidelines
>>> here: https://wiki.openstack.org/wiki/Documentation/ReviewGuidelines so
>>> feel free to enhance those to help solve the problem.
>>> Before we do this, I suggest that we experiment a bit with the workflow.
>>> Nick, could you show us a few examples how this would work, please?
>>
>> Basically, as far as the ReviewGuidelines, we would add a new type of
>> change, making the three possibilities "Objective", "Subjective", and
>> "Cosmetic/Convention".  So for a (probably silly, but demonstrative)
>> example:
>>
>> -- You submit a change that adds a new section talking about how to add,
>> say, a new driver for Cinder, and everywhere you should have "OpenStack
>> Block Storage" you just say "cinder".  You a
>> -- I review your change, and I see that everything is fine, except for
>> those 17 instances of "cinder".
>> -- From here it can go one of three ways:
>> -- 1) I comment that I'm going to fix it (so you don't spend time on
>> it), then correct them and submit a patch, and +1.
>> -- 2) I mark them, and you fix them and resubmit.  I +1.
>> -- 3) I let you know that it needs to be fixed and:
>> -- -- 3a) you submit a bug and drop it into comments, then I +1 or
>> -- -- 3b) I submit a bug, then +1

I'm fine with having those three options.

Personally for 17 instances, I would go in most cases for 2 - with a
note for a first time contributor asking whether that person needs any
help or whether I should take over - like I did with
https://review.openstack.org/#/c/158161/

> So, this isn't totally insane. From where I'm sitting though, I'd vastly
> prefer to see 1 and 2 happen in 90%+ cases. I think we need to be having

I agree with the 90 %+ cases.

> a broader discussion about review rigour in general, though, so we can
> better identify where a -1 isn't warranted, where a reviewer should do a
> quick fix on someone else's patchset, or where a new bug is a better way
> of fixing issues.
> 
> I know when I started working on OpenStack docs, doing reviews was quite
> daunting, mostly because I didn't have a good idea what level of review
> rigour was expected. Here's some notes I wrote about that at the time:
> 
>> A note on review rigour: There are very few guidelines about what
>> consists of a successful patch, but the general approach seems to be
>> that if it's technically accurate and better than the existing
>> content, then it should be approved. The main things to look for:
>> General spelling and grammar.
>> Technical accuracy. Where possible, test commands on your own VM to
>> make sure they're accurate. Check any related bugs, and the Disqus
>> comments on the doc site to see if there's anything else you might
>> need to take into account.
>> The 'is it better than what we have already' test. Check the diff, or
>> go look at the current document on the doc site, and determine if the
>> changes are an improvement.
>> Provide corrections in-line (double click on the offending line in the
>> diff viewer to write your suggested changes) for the author to fix if
>> there's more than a couple of errors. If there's just one or two
>> really minor changes (or in a situation where the writer is either ESL
>> or could be otherwise unable to improve the doc themselves), consider
>> checking out the patch and editing it quickly yourself. Be nice.
> 
> Hopefully this can start a broader conversation about what is/isn't
> expected from reviewers :)

Good notes,

Andreas
-- 
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
       Graham Norton, HRB 21284 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126