openstack team mailing list archive

[Andy Smith <andyster@xxxxxxxxx>]

On Mon, Feb 21, 2011 at 12:52 AM, Thierry Carrez <thierry@xxxxxxxxxxxxx>wrote:

> Andy Smith wrote:
> > I have some anecdotal evidence to add to this from my time at Google:
> >
> > (1) At Google in all reality you spent at least 2 days a week pretty
> > much only participating in code review and mailing list responses. This
> > is due to a couple things, but mostly because code review is taken
> > extremely seriously, the reviewer of the code has as much responsibility
> > for what lands as the person writing the code, their name (or names) go
> > in change commit. If that code creates a problem it is up to all people
> > involved in that process to quickly come up with a resolution.
> >
> > That responsibility leads to some other great things:
> >  * Lessening of self-defensiveness / personal investment in code: the
> > code is not yours, it is multiple people's.
> >  * You also always have at least one "buddy" who can back up the
> > decisions that were made, if you are not around to argue a point that
> > person probably can, and no attacks can ever be leveled at you
> personally.
>
> We should definitely add the nova-core reviewers LPids to the commit
> message, in order to encourage that spirit. Something like:
>
> [r=soren,termie] Rest of commit message...
>

Anybody know how hard it is to incorporate this into our tarmac?


>
> > (2) At Google you generally have to give explicit targets for who should
> > be your code reviewer. This prevents some tragedy of the commons
> > behaviors (when there is nobody assigned everybody expects somebody else
> > to do it).
> >
> > This also leads to people who are defacto (or explicit) leaders for
> > certain sections of code. For example, when fixing a bug on a section of
> > code you are not usually working in it is common to ask around on IRC
> > (or just the office) to find out who knows most about that area and
> > should do the review.
>
> This can easily be done by specifying reviewers when you do the branch
> merge proposal.
>

Sure, we just need to remove it automatically selecting "Nova Core" and
likewise as a reviewer for you so that people are forced to explicitly add a
reviewer.


>
> > (3) At Google one of the first things that new developers do is read
> > through a couple nicely written documents on how to conduct code
> > reviews, what your responsibilities are when doing code review, and some
> > ways to make sure your tone comes off constructively.
> >
> > This keeps everybody on most of the same page and helps acclimatize
> > people to social interaction related to coding.
>
> +1 on reference docs :)
>
> > I think adopting these behaviors would be in our best interest as a
> > project, if that sounds good I am willing to take the time to generate
> > the initial draft of the document and get the appropriate configurations
> > / code updated to support tracking reviewers and requiring explicit
> > reviewers.
>
> For (1), ideally Tarmac would support rewriting the commit message in
> transit to include those names... (2) is already doable with our current
> toolset. For (3), just let us know when the wiki draft is up so that we
> can participate in improving it.
>
> Cheers,
>
> --
> Thierry Carrez (ttx)
> Release Manager, OpenStack
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openstack
> Post to     : openstack@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openstack
> More help   : https://help.launchpad.net/ListHelp
>