Review Process Experiment (1 LGTM)

[Nate Finch]

I accidentally only replied to John with this earlier:

I like the shared responsibility and the review of existing code, but I do
lament the loss of that extra pair of eyes on the code. Even in my short
time at Canonical, I have almost always seen the second review turn up
things the first review missed. This is likely due to each reviewer valuing
different things and therefore looking at those things more carefully, and
probably also that the first one picked up the easy stuff, so the second
one could ignore that and look for more subtle problems. Granted, most of
the time it's not incorrect code, just something that could be tweaked to
be better, but that still gives us higher quality code overall.

I do think that putting a higher bar on LGTMs is a good thing, and
hopefully that can help keep the quality high.


On Mon, Aug 19, 2013 at 2:47 AM, John Arbash Meinel
<john at arbash-meinel.com>wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> This is the email I want to send to juju-dev, I just want to run it
> past everyone to make sure it fits what we agreed to.
>
>
>
> One of the things we discussed in the Isle of Man was tweaking our
> review process.
>
> We want to try an experiment, where we change how we do code reviews a
> little. We want to try it for about a month, and then at the end of it
> we evaluate if we want to keep doing it. The short summary is
> switching to 1 LGTM for landing patches, and doing a weekly shared
> review of parts of the codebase.
>
> The specific changes are:
>
> 1) Only one LGTM needed to land code.
>
>    a) This raises the stakes slightly on actually voting LGTM. We'd
>       like it to mean "I understand this patch enough that if there was
>       a problem, I would be responsible for it."
>
>       It is still great to get comments on a proposal, and have
>       conversations about whether you actually understand something.
>
>    b) If you want a specific person to review something you are unclear
>       about, *ask* for them to help. If you are unsure it really helps
>       to point it out in your merge request overview, rather than
>       needing the reviewer to discover it.
>
>    c) This applies for both the submitter and the reviewer. If you want
>       a second review, you are encouraged to just ask.
>
>    d) Also, we should feel fine sending comments about something you've
>       seen even if that code has already landed.
>
>
> 2) We'll start doing a weekly shared review of existing code.
>
>    a) The idea is that rather than spend resources having 3 people look
>       at a new patch (1 writer + 2 LGTM), spend that time auditing the
>       code we already have.
>
>       A lot of the benefits from code review are exposing stuff to
>       other people, but we only get that exposure for the code that is
>       being actively developed, and miss the stuff that you might have
>       to maintain later.
>
>    b) In the first pass, we want to give a file for people to look at.
>       They should spend about an hour going over it, and then we'll
>       spend an hour as a group talking about it. We'll probably take
>       one of the daily standup calls for this. I think we'll need to
>       split it so that Tim, Ian and Andrew can do one in a
>       work-friendly time. We may grow this into a whole package, we're
>       just ballparking for how much time this will take.
>
>    c) We expect that this will uncover a fair amount of tech debt. That
>       doesn't all need to be fixed immediately, though. If it is
>       trivial (can be fixed in the time to review it), just fix it. But
>       mostly we want to make sure people understand the code base a bit
>       more, and having them be aware that some bits of existing code
>       aren't great as a template is still valuable.
>
> So we'd like to switch to this going forward, and re-evaluate if we
> feel it has been a positive change on Sept 12th.
>
> John
> =:->
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (Cygwin)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlIRv4gACgkQJdeBCYSNAAPIDACeL4OBNMyjypq/jKYkSJNOsEhW
> mRgAn2WkuFE7oKmo9lS5tw4yoiiSe7cg
> =Uz07
> -----END PGP SIGNATURE-----
>
> --
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20130819/c50cd36d/attachment.html>