<div dir="ltr">Sounds really good. I especially like the idea of a shared review of the existing code.<div><br></div><div>mue</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 19, 2013 at 11:02 AM, Dimiter Naydenov <span dir="ltr"><<a href="mailto:dimiter.naydenov@canonical.com" target="_blank">dimiter.naydenov@canonical.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">-----BEGIN PGP SIGNED MESSAGE-----<br>
Hash: SHA1<br>
<br>
</div>I see where this is aimed - speeding up the review process, which<br>
tends to stall at times for the lack of an additional LGTM. I also<br>
think that, given time it'll improve our knowledge and certainty about<br>
the codebase as a team and individually. Both of these are very good<br>
things to have, my only concern is the code quality could deteriorate<br>
in the short term due to unwillingness to "dive deeper" into more<br>
complex merge proposals, like I've seen in the past - just LGTM is NOT<br>
OK for anything more complicated than a trivial MP. But since there<br>
was a requirement for an additional LGTM, most of the time at least<br>
one reviewer did make the effort to understand the code and its effects.<br>
<br>
So I sure hope granting LGTM should now be considered carefully and<br>
it'll mean *"I _share_ the responsibility for the _quality_ of this<br>
code and *I'm willing to fix potential problems it causes"*.<br>
<br>
Other than that, a big +1 on this change.<br>
<div><div class="h5"><br>
On 19.08.2013 08:47, John Arbash Meinel wrote:<br>
> This is the email I want to send to juju-dev, I just want to run<br>
> it past everyone to make sure it fits what we agreed to.<br>
><br>
><br>
><br>
> One of the things we discussed in the Isle of Man was tweaking our<br>
> review process.<br>
><br>
> We want to try an experiment, where we change how we do code<br>
> reviews a little. We want to try it for about a month, and then at<br>
> the end of it we evaluate if we want to keep doing it. The short<br>
> summary is switching to 1 LGTM for landing patches, and doing a<br>
> weekly shared review of parts of the codebase.<br>
><br>
> The specific changes are:<br>
><br>
> 1) Only one LGTM needed to land code.<br>
><br>
> a) This raises the stakes slightly on actually voting LGTM. We'd<br>
> like it to mean "I understand this patch enough that if there was a<br>
> problem, I would be responsible for it."<br>
><br>
> It is still great to get comments on a proposal, and have<br>
> conversations about whether you actually understand something.<br>
><br>
> b) If you want a specific person to review something you are<br>
> unclear about, *ask* for them to help. If you are unsure it really<br>
> helps to point it out in your merge request overview, rather than<br>
> needing the reviewer to discover it.<br>
><br>
> c) This applies for both the submitter and the reviewer. If you<br>
> want a second review, you are encouraged to just ask.<br>
><br>
> d) Also, we should feel fine sending comments about something<br>
> you've seen even if that code has already landed.<br>
><br>
><br>
> 2) We'll start doing a weekly shared review of existing code.<br>
><br>
> a) The idea is that rather than spend resources having 3 people<br>
> look at a new patch (1 writer + 2 LGTM), spend that time auditing<br>
> the code we already have.<br>
><br>
> A lot of the benefits from code review are exposing stuff to other<br>
> people, but we only get that exposure for the code that is being<br>
> actively developed, and miss the stuff that you might have to<br>
> maintain later.<br>
><br>
> b) In the first pass, we want to give a file for people to look<br>
> at. They should spend about an hour going over it, and then we'll<br>
> spend an hour as a group talking about it. We'll probably take one<br>
> of the daily standup calls for this. I think we'll need to split it<br>
> so that Tim, Ian and Andrew can do one in a work-friendly time. We<br>
> may grow this into a whole package, we're just ballparking for how<br>
> much time this will take.<br>
><br>
> c) We expect that this will uncover a fair amount of tech debt.<br>
> That doesn't all need to be fixed immediately, though. If it is<br>
> trivial (can be fixed in the time to review it), just fix it. But<br>
> mostly we want to make sure people understand the code base a bit<br>
> more, and having them be aware that some bits of existing code<br>
> aren't great as a template is still valuable.<br>
><br>
> So we'd like to switch to this going forward, and re-evaluate if<br>
> we feel it has been a positive change on Sept 12th.<br>
><br>
> John =:-><br>
><br>
<br>
-----BEGIN PGP SIGNATURE-----<br>
</div></div>Version: GnuPG v1.4.12 (GNU/Linux)<br>
<div class="im">Comment: Using GnuPG with Thunderbird - <a href="http://www.enigmail.net/" target="_blank">http://www.enigmail.net/</a><br>
<br>
</div>iQEcBAEBAgAGBQJSEd85AAoJENzxV2TbLzHwtY4IAMDvSHNg8OURqAw855e1I0nc<br>
VO0RwHyb17hxafyRuaghZZzwzxEW0pbnBXZBauEY8QtVAJK8itxyf0/XEDVCPhhJ<br>
2BzQ2ce5sQ/SCsLXfzUUzmBlMoywHJv7fmXZGn8zYb0yMBJ5MPK9QYSvoyKx9iAs<br>
VmgtxI7BD6n2vP26hAF/PwB4+10rcF73eJgI7HZxLx0JSjZdWawgfdpbN7NjvDoQ<br>
IZYRgps2jWlTBYxAqwp2w1GyA9hbmZfOuxqNGDPKuaVHuRQO/wC1Ztl+ALpyhgZp<br>
pDsjm7ls/Iw+6VKx8htk2pJmNxFPhp2JE3ekrE2XvRdnmQys5rKKdZDB+dMucUw=<br>
=Y8mm<br>
<div class="HOEnZb"><div class="h5">-----END PGP SIGNATURE-----<br>
<br>
--<br>
Juju-dev mailing list<br>
<a href="mailto:Juju-dev@lists.ubuntu.com">Juju-dev@lists.ubuntu.com</a><br>
Modify settings or unsubscribe at: <a href="https://lists.ubuntu.com/mailman/listinfo/juju-dev" target="_blank">https://lists.ubuntu.com/mailman/listinfo/juju-dev</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>** Frank Mueller <<a href="mailto:frank.mueller@canonical.com" target="_blank">frank.mueller@canonical.com</a>></div><div>** Software Engineer - Juju Development</div>
<div>** Canonical</div>
</div>