Gerrit code review velocity

[Brad Bishop]

Hi Ed

> On Nov 15, 2017, at 4:50 PM, Tanous, Ed <ed.tanous at intel.com> wrote:
> 
> Is there anywhere that the SLAs and formats for how community code reviews are conducted?

I don’t think so.  We have been operating with 2 x +1 required and then a merge review from the maintainer (me), although I don’t always follow that if changes seem trivial.  I’d be willing entertain a stricter policy.

>  
> I’m having some concerns around the code merge velocity on both sides of the spectrum, and I thought I’d see what everyone’s thoughts were on the matter.
>  
> A great example of these concerns is below.  First and foremost, I’m not picking on this specific committer here, this seems to be a greater issue.
> https://gerrit.openbmc-project.xyz/#/c/7925/
>  
> This patchset was pushed last night, approved by a single developer,

This had two +1s from other developers before I submitted it.  What am I missing?

> and merged by the submitter <24 hours later.  Considering this is a “phosphor” reference component, changes an architecture level change (supporting multiple HWmon clients)

In my head this was a bug fix.  I’m don't understand how this is an architectural change.  I just changed the bus name format from:

xyz.openbmc_project.Hwmon<N>.Hwmon

to 

xyz.openbmc_project.Hwmon-<ID>.Hwmon1

> and doesn’t seem to have any supporting unit tests, I’m concerned at the velocity that it was moved through the progression, and into master.
>  
> Considering that we had some discussions on this specific change before, I would’ve liked to have input, and I’m sure others in the community would’ve as well, but were not given the opportunity.  

That’s all reasonable.  I support having a conversation about how code reviews are conducted and making changes to the process.  But to be honest, what would be more valuable to me is an understanding of why you view this particular change as an architectural change, and I view it as a bug fix.  That is a pretty fundamental disconnect.

Maybe to start that, would you be willing to briefly outline what you think the old architecture was, and what the new one is after this change?

> Do we have  a process for how long code review->test->merge should take, and what process it should follow?

We don’t.  My personal preference would be for this to be at the repo maintainers discretion.  I’m sure that sounds bad since I am the maintainer of nearly all of them, but I do want other people maintaining eventually and I’d still say the same thing even after it wasn’t me.

That said, I’m more concerned in building a community here, so I’m willing to submit to group-think if others feel one way or the other.

>  
> On the other end of the spectrum, I see code commits where the last activity was several months ago, and do not see anyone picking it up, nor any forward progress on merging.  Is there a process defined for abandoning commits?
>  
> If the answer to all of these questions is, “no” or “nothing written down” I’m happy to put down some of my thoughts on integration rules down in a spec-like form, and see if we can come up with something that everyone is happy with.

This is a great idea.  Thanks!

>  
> Thanks,
>  
> -Ed 


-brad