I got curious about how workflows were implemented and since the codebase is pretty small, I started digging around their code, but then found some relevant information here: https://github.com/ooyala/barkeep/issues/254

So, it appears that workflow is not part of barkeep itself since barkeep and git repos are decoupled -- this makes the default workflow post-commit, and pre-commit workflows are something that have to be scripted externally. Not sure what is involved there, but it is probably a matter of time before sample scripts / guides are up. Also, possibly, this might enable different commit-review workflows for different groups, if something like that might be useful.

Subbu.

...

On Fri, Jun 29, 2012 at 2:20 PM, Marcin Cieslak saper@saper.info wrote:

...

As seen on IRC:

https://github.com/ooyala/barkeep/wiki/Comparing-Barkeep-to-other-code-revie...

The most prominent feature of Barkeep mentioned on this page is that it was built for a post-commit review workflow. Given that the reason we moved MediaWiki to git was basically so that we could move *away* from post-commit review, I don't think using Barkeep as-is would work.

Then again, from watching the demo video (see getbarkeep.org) it looks

I assumed it was pre-commit workflow only when I first looked at Barkeep, but I found this snippet on getbarkeep.org that Roan posted.

"Barkeep is unopinionated. Use it with pre-commit or post-commit workflows, and script tools on top of it if you like. Comes with a command line client and REST APIs."

Subbu.

On Sat, Jun 30, 2012 at 4:23 PM, Faidon Liambotis faidon@wikimedia.org wrote:

Well, in the ops puppet repo though, we very often +2 commits ourselves and push them, instead of waiting for someone else to review/approve them. You could argue that it's our workflow that it's wrong, but I just think the needs for infrastructure-as-code might be different from the needs code development has.

It's like asking for pre-execution reviews of whatever you type in a shell prompt and we can all agree that's just crazy :) In a perfect world we'd stage every change in VMs where we'd do local puppet commits without reviews; then push those tested changesets into the pre-commit review system to get into production. But we're very far from that and being perfectionists just hurts us more on our daily work.

Having a proper post-commit review workflow would be much better than hacking around the system and reviewing commits ourselves. It'd also allow us to actually have post-commit reviews, something that rarely happens right now. At least I'd do that, while currently it's a PITA to do so.

Yes, ops essentially uses a post-commit workflow right now, and that makes sense for them. Gerrit doesn't support post-commit review very well so Barkeep might work better there. But other projects, such as MediaWiki core, do use real pre-commit (or pre-merge, rather) review.

Barkeep claims to work with both post- and pre-commit workflows, although the details elude me.

Per Subbu's message, you'd have to add support for pre-commit. Gerrit manages the git repositories themselves, implements fine-grained permission settings and handles gated branches both in terms of access control (no one can push directly to master, only certain people can approve things, etc.) and automatic merging.

The UI is much *much* nicer than Gerrit's. They also have a demo website, which is a pleasure to work with IMHO.

Agreed.

They also claim to have useful, relevant and configurable e-mail notifications too, in contrast to Gerrit's which are basically useless. Maybe I'm too much of a relic to prefer reading commit diffs in my mail client, rather than fancy web interfaces :)

I don't know what these "useful" e-mail notifications look like but I would love to find out. Gerrit's aren't totally useless but they're also not all that useful.

All in all, I like it very much but I don't have a broad knowledge of how people use Gerrit right now and therefore I can't form an opinion on whether it's suitable for us.

I think that depends on your definition of "us". For the puppet repo, a mix of pre-commit and post-commit (post-commit for immediate changes made by ops people, pre-commit for changes submitted by non-ops people (staff or volunteers) and for ops changes that can afford to wait for review) would probably be best. I think a number of other projects, such as the analytics repos or extensions that aren't deployed on the WMF cluster, might prefer this as well. But for MediaWiki core and deployed extensions I'm pretty sure we'll want to stick with pre-commit review.

This makes it sound like using both Gerrit and Barkeep together might work, but I don't think that's necessarily a good idea. These are the problems I imagine we'd have: * people having to learn two different tools in general: more learning, possible confusion over what lives where and when to use what * review comments on one commit are spread across two systems, and you can't see the Gerrit comments in Barkeep or vice versa * ops staff works mostly with Barkeep, non-ops and volunteers work mostly with Gerrit --> ops staff more likely to neglect Gerrit review queue

So I think it would be much better if we could have proper integration between the two, or maybe even implement pre-commit review in Barkeep. The issue ticket that Subbu found [1] has a comment suggesting how a pre-commit workflow might be implemented: * push to feature branches * run a script that polls Barkeep for approved commits in feature branches and merges them into master Problems with this approach are: * you need access controls to prevent people from pushing to master ** this can be done with something like gitolite, or even with Gerrit * Gerrit's immediate-merge-upon-approval feature is very nice ** this is easy to implement if Barkeep offers an event stream * fixing things to address review isn't handled nicely ** to fix something you'd either add a fixup commit to your branch or create a new branch with an amended commit ** in both cases the commits are separate, so the review comments are spread out and aren't linked like patchsets in Gerrit ** it's easy to accidentally approve&merge an outdated branch, or to forget all fixup commits ** (the fixup workflow is flawed in general; with the amend/new-branch workflow, every single commit in master is safe to deploy, with the fixup workflow there can be commits that are broken, don't pass the tests, or even have syntax errors)

Per the points above I think Gerrit's backend and workflow are actually more well-thought-out than people give them credit for. Only if&when Barkeep is (or is hacked to be) able to handle these things well, would I seriously consider it as an alternative to Gerrit.

Some ideas for how fixup handling might be done in a Barkeep-like system: * make people push fixup commits into the same feature branch but: ** allow viewing and reviewing/commenting/approving both the individual commits and the branch as a whole ** somehow integrate the individual and combined views so comments show up in both (Gerrit does this for patchsets) ** when merging a multi-commit branch, squash it into a single commit before merging *** this probably means a web interface for determining the commit summary of the squashed commit would be needed ** I don't know how rebasing would be handled here * alternatively, make people amend their commits and submit amended commits to myfeaturebranch/1, myfeaturebranch/2, etc. ** this is basically the Gerrit model except branch names are used to link commits rather than Change-Ids ** the same integration described above would be needed, but if desired it could be more Gerrit-like ** handling rebasing is not a problem ** handling dependent commits (if you amend a commit, other commits that depend on it need to be rebased) is just as problematic as in Gerrit, although this could be mostly alleviated by better UI support (offer automatic rebasing in the UI; if that conflicts, tell the user to rebase manually and give them the required commands for easy copy-pasting)

My ideal system would probably be the amend-into-numbered-branch model but with better handling for series of dependent commits based on the fixup model.

At least there's some competition in the space and other people having the same problems as (at least) I do, that's good :)

Yes, I'm really happy Barkeep was made (and brought to my attention), and hopefully it can drive innovation in the direction we need it to go in.

Roan

[1] https://github.com/ooyala/barkeep/issues/254

On Sat, Jun 30, 2012 at 11:53 PM, Antoine Musso hashar+wmf@free.fr wrote:

Roan Kattouw wrote:

...

Yes, ops essentially uses a post-commit workflow right now, and that makes sense for them.

ops also uses pre-commit review for non-ops people :-]

Yeah, that's right. What I meant to say (and thought I had said in some form later in that message) was that the puppet repo has post-commit review for most changes by ops staff, and pre-commit review for everything else (non-ops staff, volunteers, and certain changes by ops staff in some cases).

Roan

Some notes on the UI interface:

(i) it doesn't allow to review the commit message

(ii) the github-like comment and diff UI is nice, but the diffs aren't as informative: they don't seem to highlight extra whitespace and tabs as well as Gerrit.

(iii) the interface showing the files is less informative: no list of files already checked, all changes in one page (wonderful for the commits editing 10+ files).

(iv) it doesn't seem to support successive patch sets for one change, so maybe it's more post-commit centered than workflow-agnostic. So what should we do? Create another commit to fix the commit? Where will be the comments? On several pages? How will there be linked?

(v) The work to adapt the software from pre-commit to post-commit. I don't understand how to fix a review.

(vi) I don't understand how to test a change (yes it's a post-commit workflow, so just git pull, but in the case we transform Barkeep to a pre-commit...).

Globally, I don't share the general enthusiasm and feeling about the UI ; I really think the Gerrit interface is more functional and more ergonomic, where Barkeep only gives a "2.0 fresh look".

If it's only an interface issue, I would strongly advice to follow the rule "When it's not broken, don't fix it" and keep Gerrit instead to waste time and resources to hack Barkeep to have it suiting our pre-commit workflow need.

On Mon, Jul 2, 2012 at 12:06 AM, Ryan Lane rlane32@gmail.com wrote:

...

Yeah, that's right. What I meant to say (and thought I had said in some form later in that message) was that the puppet repo has post-commit review for most changes by ops staff, and pre-commit review for everything else (non-ops staff, volunteers, and certain changes by ops staff in some cases).

And we'd gladly take better tools for doing post-commit review. Gerrit handles this very poorly. Just having free-form tags in Gerrit would likely fix this for our use case, though.

Saved searches would be amazing.

• Ryan

---

Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

I became curious with these statements regarding self-review (committer==reviewer) and so I ran a couple of queries against the gerrit database to see how often this occurs:

1. For the puppet repo, 84.1% of the commits is self-reviewed.
2. For the mediawiki core repo, 27.9%  of the commits is self-reviewed.

I'm actually very surprised to see such a high number for MediaWiki core. Why is this the case?

When you say committer==reviewer, so do you mean the reviewer did a +2 and merged? Does this take into account committers that also just added comments?

3. For the mediawiki extensions repos, 67.8%  of the commits is self-reviewed.

This doesn't surprise me at all. I have a *really* hard time getting my extensions reviewed and I work for the foundation. I have to beg for reviews, and even then it can take up to a couple weeks sometimes. It's problematic.

That said, I don't necessarily think my extensions need pre-merge review. I'd really like Gerrit to support post-merge review in some fashion.

I think we need to take a step back from a tool-focused discussion and first hash out what our commit workflows are / should be. In particular:

1. Should there be one commit workflow that applies to all teams? Looking at current practise, the answer seems to be no but I am curious to hear what other people think. If the answer is that it's okay for different teams to have different commit workflows, then we should also look for tools that support this.

Things that are going into production should be reviewed, preferably pre-merge. It should definitely be reviewed before deployment. Insecure code on a wikimedia.org domain has the ability to affect everything else too.

2. If self-review is so prevalent, does that mean that the pre-commit review workflow has failed?

Self-review is so prevalent in puppet because it's very difficult to do operations any other way. We occasionally do 50-100 deployments a day. Labs isn't a fully functional replica of everything in production yet, either, so we have no way to stage deployments, which means we can't wait on reviews.

That said, pre-merge review is working exactly as intended. The fact that nearly 20% of the merges are reviewed shows exactly how well it's working. If you take a look at that more closely, I'm betting you'll see that those commits are almost completely outside of the operations staff. That's an amazingly huge win for a team that had 0 merged commits outside of the operations staff. This process simply wouldn't work without pre-merge review.

- Ryan

Yeah, I don't think Ops is proud of this, but from my understanding, it's very difficult to develop for puppet without committing and seeing what happens.  It's possible, but it's definitely not as productive.

That's not true anymore. It's been possible to fully test puppet manifests before committing for over a month, now. Nearly two weeks ago, we even got rid of the test branch in favor of pushing fully finished changes into the production branch. This assumes that this piece of infrastructure is in Labs, of course.

- Ryan

On Mon, Jul 2, 2012 at 9:59 PM, Rob Lanphier robla@wikimedia.org wrote:

On Mon, Jul 2, 2012 at 9:19 PM, Diederik van Liere dvanliere@gmail.com wrote:

...

I became curious with these statements regarding self-review (committer==reviewer) and so I ran a couple of queries against the gerrit database to see how often this occurs:

1. For the puppet repo, 84.1% of the commits is self-reviewed.

Yeah, I don't think Ops is proud of this, but from my understanding, it's very difficult to develop for puppet without committing and seeing what happens. It's possible, but it's definitely not as productive.

I would agree with Ryan and say that it's not that we're not proud of this, it's that we have a different workflow. There's a lot of repetitive style work in our job (putting new servers in puppet and dhcp files, for example). These minor commits don't need any major review. Major changes can be tested in labs, usually have someone else check them out, and for many changes the worst breakage that happens is that puppet stops running(instead of a dead site).

Leslie

I have explored how to make a gerrit-substitute and it doesn't seem that hard. Initially, I planned to base on gitloite, but its workflow doesn't suit with gerrit. Basically, you would use a hook to rewrite the push references, and then run a script to register it in the db and UI (I did some simple tests). You just need some glue to make the UI perform git changes (merges) and a fancy display of the changes. Things like command filtering are tricky, but could be obtained from gitolite. The best of making a gerrit-similar solution is that it would be able to show gerrit history, thus fully replacing it (no need to hunt for old changesets in Special:Code/Gerrit/newtool). I expect a functonal gerrit clone could be made in a few weeks. (disclaimer: I don't have such weeks available now, while I may advance it on the future, don't expect me to come with an alternative tomorrow, OTOH if someone wants to give it a go...)