Open Stack

On 5/22/19 7:12 AM, Alexandra Settle wrote:
> Replying to the last thread and top posting both on purpose as I want to 
> point out something slightly differently but I am taking this platform 
> as a way to create further discussion...
> 
> The topic of +1 and -1 was brought up in a number of times at the most 
> recent PTG. In the past it has been somewhat obvious that we HAVE had 
> individuals who provide a +1 or -1 to improve their review stats. Now, 
> whilst we *now* have the guide to reviewing as Julia pointed out [0] we 
> originally created a community that kind of had to make up the meanings 
> behind a +1 and -1. I am speaking generally here, some projects 
> definitely had defined rules as to what these reviews meant, but not all 
> did. This obviously left this very open to interpretation.
> 
> To become a core review in many projects, the barrier to entry was 
> general "you must provide quality reviews". That's great, except what 
> ended up happening is we had waves of both individuals providing mass 
> +1's and -1's. Neither were quality.
> 
> When we (the docs team) were looking at who to offer core 
> responsibilities to, we used to look specifically at the amount of +1's 
> and -1's. At that point in time, a -1 was seen to be a review of "higher 
> quality" because it meant somehow that the individual's critical review, 
> automatically meant "critical thinking". All this did was create a 
> negative review culture ("a -1 is better than a +1"). I know I used to 
> be highly conscious of how many +1's I offered before I became core, I 
> would often avoid providing a +1 even if the patch was fine so my stats 
> would not be messed up. Which is pretty messed up.

Definitely, and this is why I avoid mentioning review stats when 
proposing someone as core. In fact, I barely look at review stats when 
making the decision. Basically I just verify that they've been doing 
reviews, and if so I move on.

It's far more important to me that someone demonstrates an understanding 
of Oslo and OpenStack. -1's are one way to do that, but they're not the 
only way. Nit picky -1's are _not_ a way to do that, and in fact they 
demonstrate a lack of understanding of where we're trying to go as a 
community. Followup patches to fix the nits are terrific way to 
ingratiate yourself though *hint hint*. :-)

I know I've seen blog posts, but do we have any official-ish 
documentation of best practices for becoming a core reviewer in 
OpenStack? I realize some of it is project-specific, but it seems like 
there would be some commonalities we could document.

> 
> We have the review document in the project team guide, but what are we 
> doing to socialise this? This thread currently is divulging into 
> personal opinions and experiences (myself included here) but it looks 
> like we need to work on some actions.
> 
> A few questions:
> 
>  1. Who knew about the "How to Review Changes the OpenStack Way"
>     document before this thread?

I did, but I'm pretty sure I was involved in the discussion that led to 
the creation of it so I may not be typical. :-)

>      1. If you did, how did you find the content? Did you find it easy
>         to understand and follow?
>      2. If you did not, would you have found this document helpful when
>         you first started reviewing?
>  2. PTLs - do you share this document with new contributors that are
>     reappearing?

I haven't been, but I will try to start.

Also I need to update 
https://docs.openstack.org/project-team-guide/oslo.html It still talks 
about oslo-incubator. o.O

>  3. Is this socialised in the Upstream Institute meeting? (@diablo)
> 
> I don't expect a flood of answers to these questions, but it's important 
> that these are on our mind.
> 
> Cheers,
> 
> Alex
> 
> IRC: asettle
> Twitter: @dewsday
> 
> [0]: 
> https://docs.openstack.org/project-team-guide/review-the-openstack-way.html
> 
> On 22/05/2019 11:02, Natal Ngétal wrote:
>> On Tue, May 21, 2019 at 3:33 PM Brian Rosmaita
>> <rosmaita.fossdev at gmail.com>  wrote:
>>> A recent spate of +1 reviews with no comments on patches has put me into
>>> grumpy-old-man mode.
>> What do you mean by this? I make several code review and each time I
>> add +1 that will mean I have made a code review.
>>
>>> A +1 with no comments is completely useless (unless you have a review on
>>> a previous patch set with comments that have been addressed by the
>>> author).  I already know you're a smart person (you figured out how to
>>> sign the CLA and navigate gerrit -- lots of people can't or won't do
>>> that), but all your non-comment +1 tells me is that you are in favor of
>>> the patch.  That doesn't give me any information, because I already know
>>> that the author is in favor of the patch, so that makes two of you out
>>> of about 1,168 reviewers.  That's not exactly a groundswell of support.
>> I don't make code review to improve my statics, however sometimes I
>> add a +1 and I have don't saw an error and another saw an error.
>> That doesn't means I have don't read the code, sometimes during a code
>> review we can also make a fail. This why it's good to have many
>> different person make the code review on a patch.
>>
>>> When you post your +1, please leave a comment explaining why you
>>> approve, or at least what in particular you looked at in the patch that
>>> gave you a favorable impression.  This whole open source community thing
>>> is a collaborative effort, so please collaborate!  You comment does not
>>> have to be profound.  Even just saying that you checked that the release
>>> note or docs on the patch rendered correctly in HTML is very helpful.
>> For me a +1 without comment is ok. The +1 is implicit that mean looks
>> good to merge, but must be review by another person. Impose to add a
>> comment for each review, it's for me a nonsense and a bad idea.
>> Which type of comment do you will? I mean if it's only to add in the
>> comment, looks good to merge, that change nothing.
>>
>>> The same thing goes for leaving a -1 on a patch.  Don't just drop a -1
>>> bomb with no explanation.  The kind of review that will put you on track
>>> for becoming core in a project is what johnthetubaguy calls a
>>> "thoughtful -1", that is, a negative review that clearly explains what
>>> the problem is and points the author in a good direction to fix it.
>> I totally agree a -1 must be coming with a comment, it's a nonsense to
>> have a -1 without explanation.
>>