Review Process Experiment (1 LGTM)

[Frank Mueller]

Sounds really good. I especially like the idea of a shared review of the
existing code.

mue


On Mon, Aug 19, 2013 at 11:02 AM, Dimiter Naydenov <
dimiter.naydenov at canonical.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> I see where this is aimed - speeding up the review process, which
> tends to stall at times for the lack of an additional LGTM. I also
> think that, given time it'll improve our knowledge and certainty about
> the codebase as a team and individually. Both of these are very good
> things to have, my only concern is the code quality could deteriorate
> in the short term due to unwillingness to "dive deeper" into more
> complex merge proposals, like I've seen in the past - just LGTM is NOT
> OK for anything more complicated than a trivial MP. But since there
> was a requirement for an additional LGTM, most of the time at least
> one reviewer did make the effort to understand the code and its effects.
>
> So I sure hope granting LGTM should now be considered carefully and
> it'll mean *"I _share_ the responsibility for the _quality_ of this
> code and *I'm willing to fix potential problems it causes"*.
>
> Other than that, a big +1 on this change.
>
> On 19.08.2013 08:47, John Arbash Meinel wrote:
> > 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.12 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQEcBAEBAgAGBQJSEd85AAoJENzxV2TbLzHwtY4IAMDvSHNg8OURqAw855e1I0nc
> VO0RwHyb17hxafyRuaghZZzwzxEW0pbnBXZBauEY8QtVAJK8itxyf0/XEDVCPhhJ
> 2BzQ2ce5sQ/SCsLXfzUUzmBlMoywHJv7fmXZGn8zYb0yMBJ5MPK9QYSvoyKx9iAs
> VmgtxI7BD6n2vP26hAF/PwB4+10rcF73eJgI7HZxLx0JSjZdWawgfdpbN7NjvDoQ
> IZYRgps2jWlTBYxAqwp2w1GyA9hbmZfOuxqNGDPKuaVHuRQO/wC1Ztl+ALpyhgZp
> pDsjm7ls/Iw+6VKx8htk2pJmNxFPhp2JE3ekrE2XvRdnmQys5rKKdZDB+dMucUw=
> =Y8mm
> -----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
>



-- 
** Frank Mueller <frank.mueller at canonical.com>
** Software Engineer - Juju Development
** Canonical
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20130819/bf16e596/attachment-0001.html>