Improving Wikimedia's Code Review process

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Improving Wikimedia's Code Review process

Andre Klapper-2
Hey everybody!

At the Wikimedia Developer Summit there was a session about "Making
Code Review not suck" [1]. 
The outcome are Phabricator (sub)tasks of the task "Define potential
actions to reduce code review queues and waiting times" in
  https://phabricator.wikimedia.org/T101686

The following potential actions items have been identified:

https://phabricator.wikimedia.org/T129842 : 
  Decide whether to have a "Code Review" committee / working group
https://phabricator.wikimedia.org/T129067 : 
  Agree on and document a structured, standardized approach for 
  reviewing code contributions
https://phabricator.wikimedia.org/T129068 : 
  Improve code contribution guidelines for patch authors
https://phabricator.wikimedia.org/T128370 : 
  Update ownership list on [[mw:Developers/Maintainers]]
https://phabricator.wikimedia.org/T128371 : 
  Set up Code Review office hours
https://phabricator.wikimedia.org/T128372 : 
  Document use of Owners in Phabricator and advertise it
https://phabricator.wikimedia.org/T115852 : 
  Fix unclear maintenance
responsibilities for some parts of MediaWiki 
  core repository

Discussion on each of those proposals is welcomed in the corresponding
tasks!

Thanks,
andre

[1] https://phabricator.wikimedia.org/T114419
--
Andre Klapper | Wikimedia Bugwrangler
http://blogs.gnome.org/aklapper/



_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving Wikimedia's Code Review process

Daniel Kinzler
There will be an RFC meeting tonight about this:
https://phabricator.wikimedia.org/E148

One thing that I don't remember coming up during the summit is:

Can we perhaps get all the people with +1 rights to use them and actually review
stuff? So that people with +2 rights can look at things that already have a +1,
and can thus avoid getting swamped with lots of patches with lots of low grade
issues?

Where would this idea fit into the zoo of tickets we have on the subject?

Am 15.03.2016 um 17:11 schrieb Andre Klapper:

> Hey everybody!
>
> At the Wikimedia Developer Summit there was a session about "Making
> Code Review not suck" [1].
> The outcome are Phabricator (sub)tasks of the task "Define potential
> actions to reduce code review queues and waiting times" in
>   https://phabricator.wikimedia.org/T101686
>
> The following potential actions items have been identified:
>
> * https://phabricator.wikimedia.org/T129842 :
>   Decide whether to have a "Code Review" committee / working group
> * https://phabricator.wikimedia.org/T129067 :
>   Agree on and document a structured, standardized approach for
>   reviewing code contributions
> * https://phabricator.wikimedia.org/T129068 :
>   Improve code contribution guidelines for patch authors
> * https://phabricator.wikimedia.org/T128370 :
>   Update ownership list on [[mw:Developers/Maintainers]]
> * https://phabricator.wikimedia.org/T128371 :
>   Set up Code Review office hours
> * https://phabricator.wikimedia.org/T128372 :
>   Document use of Owners in Phabricator and advertise it
> * https://phabricator.wikimedia.org/T115852 :
>   Fix unclear maintenance
> responsibilities for some parts of MediaWiki
>   core repository
>
> Discussion on each of those proposals is welcomed in the corresponding
> tasks!
>
> Thanks,
> andre
>
> [1] https://phabricator.wikimedia.org/T114419
>


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving Wikimedia's Code Review process

Jon Robson-2
We have two swat windows every day. It's magical... I post a request for a
deploy on a Wiki page and someone deploys it.

Could we try a similar thing with code review. Code review window (maximum
1 patch per person) and have a group of +2ers look at a maximum set of
patches?

It would need a few more rules than that and a bit of tweaking but seems
like a good experiment. I'd sign up to help it if it was a maximum 2
windows for me a week.
On 16 Mar 2016 12:50 p.m., "Daniel Kinzler" <[hidden email]> wrote:

> There will be an RFC meeting tonight about this:
> https://phabricator.wikimedia.org/E148
>
> One thing that I don't remember coming up during the summit is:
>
> Can we perhaps get all the people with +1 rights to use them and actually
> review
> stuff? So that people with +2 rights can look at things that already have
> a +1,
> and can thus avoid getting swamped with lots of patches with lots of low
> grade
> issues?
>
> Where would this idea fit into the zoo of tickets we have on the subject?
>
> Am 15.03.2016 um 17:11 schrieb Andre Klapper:
> > Hey everybody!
> >
> > At the Wikimedia Developer Summit there was a session about "Making
> > Code Review not suck" [1].
> > The outcome are Phabricator (sub)tasks of the task "Define potential
> > actions to reduce code review queues and waiting times" in
> >   https://phabricator.wikimedia.org/T101686
> >
> > The following potential actions items have been identified:
> >
> > * https://phabricator.wikimedia.org/T129842 :
> >   Decide whether to have a "Code Review" committee / working group
> > * https://phabricator.wikimedia.org/T129067 :
> >   Agree on and document a structured, standardized approach for
> >   reviewing code contributions
> > * https://phabricator.wikimedia.org/T129068 :
> >   Improve code contribution guidelines for patch authors
> > * https://phabricator.wikimedia.org/T128370 :
> >   Update ownership list on [[mw:Developers/Maintainers]]
> > * https://phabricator.wikimedia.org/T128371 :
> >   Set up Code Review office hours
> > * https://phabricator.wikimedia.org/T128372 :
> >   Document use of Owners in Phabricator and advertise it
> > * https://phabricator.wikimedia.org/T115852 :
> >   Fix unclear maintenance
> > responsibilities for some parts of MediaWiki
> >   core repository
> >
> > Discussion on each of those proposals is welcomed in the corresponding
> > tasks!
> >
> > Thanks,
> > andre
> >
> > [1] https://phabricator.wikimedia.org/T114419
> >
>
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving Wikimedia's Code Review process

Rob Lanphier-4
On Wed, Mar 16, 2016 at 3:47 PM, Jon Robson <[hidden email]> wrote:

> We have two swat windows every day. It's magical... I post a request for a
> deploy on a Wiki page and someone deploys it.
>
> Could we try a similar thing with code review. Code review window (maximum
> 1 patch per person) and have a group of +2ers look at a maximum set of
> patches?
>

Hi Jon,

The SWAT comparison is a nice one!  Could you drop a comment on T128371
<https://phabricator.wikimedia.org/T128371> (Code Review office hours) to
that effect?

Rob
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving Wikimedia's Code Review process

Antoine Musso-3
In reply to this post by Daniel Kinzler
Le 16/03/2016 20:49, Daniel Kinzler a écrit :

> There will be an RFC meeting tonight about this:
> https://phabricator.wikimedia.org/E148
>
> One thing that I don't remember coming up during the summit is:
>
> Can we perhaps get all the people with +1 rights to use them and actually review
> stuff? So that people with +2 rights can look at things that already have a +1,
> and can thus avoid getting swamped with lots of patches with lots of low grade
> issues?
>
> Where would this idea fit into the zoo of tickets we have on the subject?

Hello,

I once proposed to get an additional tier in our code review system, a
level that would approve what has been reviewed.  I can not find the
link though, but I believe I got the inspiration from OpenStack. Their
workflow is:

Anyone can Code-Review +1
Core reviewers do Code-Review +2
Maintainers Approve +1
Release manager deal with branches and accepting hotfixes.

They have a convention or a Gerrit enforcement that a changes need two
CR+2 before it get approved.   Once Approved +1 , the change get merged.

What does it mean for a maintainer?  They can craft a query to look for
changes that have received a few CR+2, focus on the release quality and
almost blindly approve changes in bulk.   I am part of such group and it
makes it very easy to do the final review and land patches.

Drawback, as a dev you need to seek +2 from a couple core reviewers...

That got shoot down as not being the wiki way.  Instead we are much more
liberal in giving CR+2 right on the repo, in turn trusting people with
such privilege to not CR+2 patches on code they are unfamiliar with.   I
think it is a good compromise.


If we had some kind of dashboards to list patches that are ready enough,
got a fair amount of CR votes,  that would probably make life easier for
people landing changes.  I myself have bookmarked a couple Gerrit queries:

A) for changes on which I am a reviewer and haven't voted yet:

 is:open reviewer:self NOT owner:self label:code-review=0,self

That is the first page I hit every morning which list reviews I need to
do for others.

B) Changes reviewed with no CR-1/-2 and no verified -1 (Jenkins fail)

is:open reviewer:self NOT owner:self label:code-review=0,self
label:code-review=1 NOT label:code-review=-1 NOT label:code-review=-2
NOT label:verified<=0

That is more or less changes I can CR+2 right away.

--
Antoine "hashar" Musso


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Code review office hours (was Re: Improving Wikimedia's Code Review process)

Greg Grossmeier-2
In reply to this post by Jon Robson-2
Changing subject to focus on the code-review office hours idea.

Also, a request at the end for people with +2 to indicate if they would
be willing to participate in an experiment or not :)

<quote name="Jon Robson" date="2016-03-16" time="15:47:49 -0700">
> We have two swat windows every day. It's magical... I post a request for a
> deploy on a Wiki page and someone deploys it.

:) glad you like it

> Could we try a similar thing with code review. Code review window (maximum
> 1 patch per person) and have a group of +2ers look at a maximum set of
> patches?
>
> It would need a few more rules than that and a bit of tweaking but seems
> like a good experiment. I'd sign up to help it if it was a maximum 2
> windows for me a week.

That sounds like an interesting idea indeed, Jon.

An action item from me from the DevSummit was
https://phabricator.wikimedia.org/T128371, which is basically "setup
code-review office hours" without much more detail than that (it was a
drive-by idea during one of the sessions that I volunteered to follow-up
with).

My initial idea was very minimal (basically just a time and a virtual
place to do code-review together) but adding in the explicit support of
+2ers helping merge ready patches during the time gives it more
effectiveness.

The hardest part will, I assume, be getting enough people with commit
rights to volunteer for at least one day/week.

Any one reading this far with +2 willing to? If so, please comment on
the task (to save the mailing list):
https://phabricator.wikimedia.org/T128371

Thanks,

Greg

--
| Greg Grossmeier            GPG: B2FA 27B1 F7EB D327 6B8E |
| identi.ca: @greg                A18D 1138 8E47 FAC8 1C7D |

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Code review office hours (was Re: Improving Wikimedia's Code Review process)

Sébastien Santoro
I've added a poll in the task description as an attempt to get
a synthetic view of who is volunteer to review.

You can vote at https://phabricator.wikimedia.org/T128371
or directly at https://phabricator.wikimedia.org/V9.

On Tue, Mar 29, 2016 at 11:22 PM, Greg Grossmeier <[hidden email]> wrote:

> Changing subject to focus on the code-review office hours idea.
>
> Also, a request at the end for people with +2 to indicate if they would
> be willing to participate in an experiment or not :)
>
> <quote name="Jon Robson" date="2016-03-16" time="15:47:49 -0700">
>> We have two swat windows every day. It's magical... I post a request for a
>> deploy on a Wiki page and someone deploys it.
>
> :) glad you like it
>
>> Could we try a similar thing with code review. Code review window (maximum
>> 1 patch per person) and have a group of +2ers look at a maximum set of
>> patches?
>>
>> It would need a few more rules than that and a bit of tweaking but seems
>> like a good experiment. I'd sign up to help it if it was a maximum 2
>> windows for me a week.
>
> That sounds like an interesting idea indeed, Jon.
>
> An action item from me from the DevSummit was
> https://phabricator.wikimedia.org/T128371, which is basically "setup
> code-review office hours" without much more detail than that (it was a
> drive-by idea during one of the sessions that I volunteered to follow-up
> with).
>
> My initial idea was very minimal (basically just a time and a virtual
> place to do code-review together) but adding in the explicit support of
> +2ers helping merge ready patches during the time gives it more
> effectiveness.
>
> The hardest part will, I assume, be getting enough people with commit
> rights to volunteer for at least one day/week.
>
> Any one reading this far with +2 willing to? If so, please comment on
> the task (to save the mailing list):
> https://phabricator.wikimedia.org/T128371
>
> Thanks,
>
> Greg
>
> --
> | Greg Grossmeier            GPG: B2FA 27B1 F7EB D327 6B8E |
> | identi.ca: @greg                A18D 1138 8E47 FAC8 1C7D |
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
Sébastien Santoro aka Dereckson
http://www.dereckson.be/

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l