Open Stack

On Fri, 2019-01-11 at 15:39 -0600, Ben Nemec wrote:
> 
> 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.
i think i partally missunderstood the proposal.
i had parsed it as replacing procedual code review -2 with a code review -1
rahter then review priority -1.

if all projects adopt review priorty going forward then that might makes sense
for those that dont i think code review -2 still makes sense.