Review Process Experiment (1 LGTM)

[Dimiter Naydenov]

-----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-----