[Gluster-devel] Gerrit review, submit type and Jenkins testing

[Atin Mukherjee]

On 11/10/2015 03:10 AM, Raghavendra Talur wrote:
> Hi,
> 
> While trying to understand how our gerrit+jenkins setup works, I
> realized of a possibility of allowing bugs to get in.
> 
> Currently, our gerrit is setup to have cherry-pick as the submit type.
> Now consider a case where:
> 
> Dev1 sends a commit B with parent commit A(A is already merged).
> Dev2 sends a commit C with parent commit A(A is already merged).
> 
> Both the patches get +2 from Jenkins.
> 
> Maintainer merges commit B from Dev1.
> Another maintainer merges commit C from Dev2.
> 
> If the two commits B and C changed code which had no merge conflicts but
> were conflicting in logic,
> then we have a master which has bugs.
> 
> If Dev3 now sends a commit D with re-based master as parent, we have the
> following cases:
> 
> 1. If bug introduced above is not racy, we have tests always failing for
> Dev3 on commit D. Tests that fail would be from components that commit B
> and C changed. Dev3 has no idea on how to fix them and has to enlist
> help from Dev1 and Dev2.
> 
> 2. If bug introduced above is racy, then there is a probability that
> Dev3 escapes from this trouble and someone else will bear it later. Even
> if the racy code is hit and test fails, Dev3 will probably re-trigger
> the tests given that they failed for a component which is not related to
> his/her code and the bug stays in code longer.
> 
> The most obvious but not practical solution to the above problem is to
> change the submit type in gerrit to "fast-forward only". It would then
> ensure that once commit B is merged, Dev2 has to re-base and re-run the
> tests on commit C with commit B as parent, before it could be merged. It
> is not practical because it will cause all patches in review to get
> re-based and re-triggered whenever a patch is merged.
> 
> A little modification to the above solution would be to 
> 
>   * change submit type to fast-forward only
>   * don't run any jenkins job on patches till they get +2 from reviewers
>   * once a +2 is given, run jenkins job on patch and automatically
>     submit it if test passes.
>   * automatically rebase all patches on review with new master and mark
>     conflict if merge conflict arises.
The overall idea looks good to me, however I'd be bit hesitant to give a
+2 before seeing the regression votes until and unless the patch is
pretty straight forwad. For me +1 sounds to be a good level to trigger
the regression. Once the regression passes, maintainers can +2 a patch
and then merge it manually. And then the 4th point follows.

Thoughts?

~Atin
> 
> As a side effect of this, Dev would now be forced to run a complete
> regression on dev machine before sending a patch for review.
> 
> Any thoughts on the above solutions or other suggestions?
> 
> Thanks,
> Raghavendra Talur
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
>