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 < [email protected]> 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 > [email protected] > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev > -- ** Frank Mueller <[email protected]> ** Software Engineer - Juju Development ** Canonical
-- Juju-dev mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
