Review-Priority for Project Repos

Ben Nemec openstack at nemebean.com
Fri Jan 11 21:39:04 UTC 2019



On 1/10/19 5:03 PM, Sean Mooney wrote:
> On Thu, 2019-01-10 at 13:42 -0600, Sean McGinnis wrote:
>>>
>>> I don't know if this was the reasoning behind Cinder's system, but I know
>>> some people object to procedural -2 because it's a big hammer to essentially
>>> say "not right now". It overloads the meaning of the vote in a potentially
>>> confusing way that requires explanation every time it's used. At least I
>>> hope procedural -2's always include a comment.
>>>
>>
>> This was exactly the reasoning. -2 is overloaded, but its primary meaning
>> was/is "we do not want this code change". It just happens that it was also a
>> convenient way to say that with "right now" at the end.
>>
>> The Review-Priority -1 is a clear way to say whether something is held because
>> it can't be merged right now due to procedural or process reasons, versus
>> something that we just don't want at all.
> for what its worth my understanding of why a procdural -2 is more correct is that this change
> cannot be merged because it has not met the procedual requirement to be considerd for this release.
> haveing received several over the years i have never seen it to carry any malaise
> or weight then the zuul pep8 job complianing about the line lenght of my code.
> with either a procedural -2  or a verify -1 from zuul my code is equally un mergeable.
> 
> the prime example being a patch that requires a spec that has not been approved.
> while most cores will not approve chage when other cores have left a -1 mistakes happen
> and the -2 does emphasise the point that even if the code is perfect under the porject
> processes this change should not be acitvly reporposed until the issue raised by the -2
> has been addressed.  In the case of a procedual -2 that typically means the spec is merge
> or the master branch opens for the next cycle.
> 
> i agree that procedural -2's can seam harsh at first glance but i have also never seen one
> left without a comment explaining why it was left. the issue with a procedural -1 is i can
> jsut resubmit the patch several times and it can get lost in the comments.

I don't think that's a problem with this new field. It sounds like 
priority -1 carries over from PS to PS.

> 
> we recently intoduced a new review priority lable
> if we really wanted to disabiguate form normal -2s then we coudl have an explcitly lable for it
> but i personally would prefer to keep procedural -2s.

To be clear, I have both used and received procedural -2's as well and 
they don't particularly bother me, but I can see where if you were 
someone who was new to the community or just a part-time contributor not 
as familiar with our processes it might be an unpleasant experience to 
see that -2 show up. As I said, I don't know that I would advocate for 
this on the basis of replacing procedural -2 alone, but if we're adding 
the category anyway I mildly prefer using it for procedural blockers in 
the future.



More information about the openstack-discuss mailing list