[Twisted-Python] major changes, release engineering, and learning cost

[Glyph Lefkowitz]

On May 26, 2010, at 5:41 AM, Laurens Van Houtven wrote:

> Major stuff could be a blueprint on Launchpad. Blueprints match a branch for the "big feature". So, we have the Twisted blueprint quantum-transmogrification and a branch lp:~lvh/twisted/quantum-transmogrification.

So, while I can definitely sympathize with a certain animosity towards trac, and I can appreciate the goals and sensibilities of launchpad, I will probably flat-out veto any required / process-driven usage of Launchpad blueprints.  Bugs, features, enhancements, etc, are all units of work that need to be tracked, and it's better to have one kind of crummy interface for tracking _everything_ than three interfaces, even three good interfaces, for tracking little bits stuff in different ways according to arbitrary distinctions.  (As someone recently opined to me, Blueprints are a giant complicated interface for pasting the URL to a Google Wave into a text field.  We might as well skip the text field and just link straight to the conversation from a Trac ticket.)

> From that branch I create a bunch of branches of review units (if it turns out it's too big, I just branch again for a new review unit). So, I want to do something with entanglement: lp:~lvh/twisted/quantum-transmogrification-entanglement, and it's good, so someone reviews and sends it back.
> 
> lp's merge proposals let you do the code review in arbitrarily small chunks. So if the thing I do next is lp:~lvh/twisted/quantum-transmogrification-ftl-travel,

lp:~lvh... isn't a verb.  What do you do with that string? :)

> and it turns out FTL travel is really really hard so I need two smaller branches lp:~lvh/twisted/quantum-transmogrification-tunnels and lp:~lvh/twisted/quantum-transmogrification-ansible. Both are good, so they get put back into ~lvh/twisted/quantum-transmogrification-ftl-travel.
> 
> Each review would verify that all children (if any) have also been reviewed. So, the final review is pretty small, as suggested :-)


The review wouldn't verify that the parent had been reviewed, though.  If you started this process by writing a bunch of code in the q-t-f-t branch, *that* code would never have been reviewed; unless q-t-f-t needs to be reviewed in its entirety before landing on trunk.  Which is precisely what I'm trying to avoid.

> This does not limit a developer's freedom to branch at will, because code review is opt-in (merge proposal), not opt-out. If you don't do it, that code in that branch isn't covered by a previous review, and must be reviewed later.

This strikes me as placing a pretty nasty burden on the reviewer.  The reviewer has to figure out if there are any commits that went only to the integration branch, isolate them, review them, get that branch into an OK-it's-reviewed state, while meanwhile other developers might be committing stuff to that branch and changing its contents, both from regular working commits and from reviewed merges.  It sounds like a nightmare.  Maybe bzr makes it easier than it sounds, but it sounds bad enough that even a big improvement would still be pretty bad ;).

> How exactly code review coverage would work is somewhat of an open question and it's the obvious failure in this system. We use it in production and it turns out to not be a problem, because people always end up doing two things:

Who is "we"?  What is "production"?  Are you talking about Twisted or a hypothetical project which uses Twisted, or a fork of Twisted on Launchpad?  Is this a hypothetical project or a real proejct?  I am super confused.

> 1) always branch at least once from the first branch off trunk (so branch off lp:~lvh/twisted/quantum-transmogrification). Net result: lp:~lvh/twisted/quantum-transmogrification only introduces code in the form of merges.

That's pretty much what I'm proposing, except I don't actually care whether they're merges or patches or individual commits, as long as they've cycled through code-review properly.

> 2) always do code review on branches being merged into your first branch off trunk (so everything merged into lp:~lvh/twisted/quantum-transmogrification has to be reviewed already)

And this is what we already do.

> Note that our merges into trunk are automagic.

(Again, who is "we", and by what mechanism are they automated?  Are you proposing that we do this, or are you stating that some other people do?)

> If it's merged into a direct branch off of trunk and it satisfies some qualities (such as full test coverage :)), it gets put into trunk, and that gets pushed to production servers. No human interaction. Scary at first, but then you realize humans were already involved in the QC process extensively at every point -- doing it this way just makes them take testing more seriously :)

Human interaction of some kind should definitely be required for Twisted.  This is not just pushing some new widget to a web site; this is potentially pushing out new APIs that need to be documented and supported to a whole ton of developers.  The whole point of the process modification I've proposed is to make sure that features arrive in releases as coherent, comprehensible whole pieces, not to allow things we can automatically verify (like docstring and test coverage) to be deferred to later merges.  These properties of the code should still be verified on every merge to the integration branch; the interesting thing about the merge to trunk is the verification that the unit is a coherent whole (and in the case of a deprecation / replacement, that the replacement is a functionally adequate upgrade).

> I think a bug would be similar except the root would not be a blueprint but a bug.

So, I'm really confused as to what the purpose of this message was - are you just describing how a similar workflow might work if we used launchpad, advocating that we switch to launchpad in order to implement this, advocating that we use launchpad for big features but *not* for other stuff, or ... what?  If you're proposing a different functional modification to the existing process, can you do it without reference to tons of launchpad-specific terminology?

Sorry if this comes off as a little flamey; I really am just confused as to what the point was.

Thanks,

-glyph