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