Review Process Experiment (1 LGTM)

[Dimiter Naydenov]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Sorry, I didn't put the proper emphasis on this:

> 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"*.

I hope this time I got it right. At least my Thunderbird makes thinks
surrounded by *asterisks* as BOLD and _underscores_ as UNDERLINED
(also /italics/ as OBLIQUE. Not sure it's only my settings or more of
you are seeing them like this.

Another thing I realized - the On-Call Reviewer of course shouldn't
feel obliged to "share the burden" and LGTM the MP if he isn't
comfortable with the proposed change.

On 19.08.2013 11:02, Dimiter Naydenov wrote:
> 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/

iQEcBAEBAgAGBQJSEnfuAAoJENzxV2TbLzHwfEMIAKjv4JgtFneKLJSU3OJ1ZOsJ
DBGnM7NsqJD851PO0nxmJ0H4a77kdXNubVJVdzGVXobliLwPAkHULJ3iINE8Bftp
OA+pnucuUo+DbSZ4Zqb9vBfphka6zsa/ChxfewkZLLSQLoo3gFte43B5XCBQWfe7
5VTqOw8bGDU4L7x9o1qmajVzA8bfUCjrs2J80IJe4+kDEyZFFzovTS+LIt/m0fvT
Bf7L/Jk51BNUqhqm0zDijEOLIR2YDoOVReSs/JzYQtrxd/u+jNvNJUMGd8A0HpYX
TiMTL1zVysQmfJ8/iAu5pSvpCYGFXx2CwYK9M8Grz0q01HUBMrMmvBEVcPLrqOc=
=/2v2
-----END PGP SIGNATURE-----