roan.kattouw at gmail
Jun 30, 2012, 8:37 PM
Post #7 of 19
(626 views)
Permalink
|
On Sat, Jun 30, 2012 at 4:23 PM, Faidon Liambotis <faidon [at] wikimedia> 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
_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
|
