[vtk-developers] VTK code review / testing / integration workflow

[Ben Boeckel]

On Wed, Aug 27, 2014 at 09:32:40 -0400, Chris Harris wrote:
>   • The development process should be enforced and supported by the review tool
>     rather than people and documents. No one wants to be the  ‘enforcer’ and
>     documents often go unread.

Agreed.

>   • GitHub/GitLab PR workflow works well with projects that have a ‘benevolent
>     dictator’ and relatively small teams, I’m not sure it scales so well when
>     you have a larger number of people with merge access where more
>     coordination is necessary.

The Rust repository juggles lots of incoming branches, but has a single
bot do all of the merges based on comments from "blessed" contributors.

>   • As well as indicating approval, it is just as important to be able indicate
>     that you don’t want something to be merged, for example a work in progress
>     branch or you don’t agree with how something should be implemented. The
>     likes of GitHub/GitLab tend to have an all or nothing approach here, if I
>     have merge privileges I can merge any PR in the project and no one can stop
>     me short of removing my privileges.

I've usually put WIP in my topic names for works-in-progress that I'd
like comments on before I dig a hole too deep on some feature. If
maintainers pull it while it still has that tag then they're not really
doing their job, IMO. If we had a mergebot, then it could just refuse
anything with and active "WIP" tag (however it ends up being specified).

>   • In my opinion no one should have push access to master, all merging should
>     be done automatically. My understanding with GitHub/GitLab is if you can
>     merge in the UI you can push to master. I have been in projects where
>     developers have push to master by mistake, it happens.

+1

>   • Long running topic with lots of commits should be the exception rather than
>     the rule. Topics that have a lot of commits are hard to review, in my
>     opinion these could be treated differently as the usefulness of the review
>     tool breaks down here. For VTK the mean number of changes in a topic: 1.88
>     and the median number of changes in a topic: 1. This to me indicates that
>     topic branch support is a nice to have rather than a necessity.

Sure, the majority are single-commits, but there's no way that all
branches belong in just one commit and that those branches are treated
as second-rate citizens in review. I view it as an absolute necessity to
support multi-commit branches gracefully. In fact, it is the larger
branches that need *more* review tool support and ease-of-use than the
small branches because they are already inherently hard to review.

One thing I *really* want is a "full branch diff" view from the tool so
that I can see the branch in its entirety. When I need to do this now, I
pull from Gerrit, but it's a pain because getting the commits from there
is unnecessarily complex (the ref names it uses aren't fetched by
default).

>   • I feel that as a part of the open source community we should be using open
>     source tools where possible. So I would add being open source as a nice to
>     have.

+1

>   • Finally I think supporting PR from GitHub is important, it's a familiar
>     workflow for many developers. However, this doesn't mean that GitHub has to
>     be our review tool. Integration is possible with other tools, for example
>     Gerrit has a GitHub plugin. I have contributed to projects that use SVN
>     through GitHub PRs :-)

A bot could pull PRs into whatever we end up using automatically with
the web hooks Github provides.

--Ben