Page MenuHomePhabricator

Code Review discussion at Wikimedia Hackathon 2019
Closed, ResolvedPublic0 Estimated Story PointsMay 19 2019

Description

Session to discuss the good, the bad, and the ugly of code review in Wikimedia today.

Let @Jrbranaa and @Aklapper be your hosts!

Sunday, May 19th, 12:00, Education center 2NP

Details

Due Date
May 19 2019, 11:00 AM

Event Timeline

Aklapper moved this task from Backlog to Sessions on the Wikimedia-Hackathon-2019 board.
Aklapper set Due Date to May 19 2019, 11:00 AM.
Restricted Application changed the subtype of this task from "Task" to "Deadline". · View Herald TranscriptMay 17 2019, 1:45 PM
Aklapper assigned this task to Jrbranaa.

Pasting here form the Etherpad:

  • DK: Daniel Kinzler
  • KH: Kosta
  • JRB: Jean-Rene
  • TK: Thiemo Kreuz
  • LM: Leszek
  • ZF: Zeljko
  • AK: Andre
  • Tgr: Gergo
  • GG: Greg Grossmeier
  • MG: MIchael Grosse

Notes:

  • Started as part of Code Health Group: https://www.mediawiki.org/wiki/Code_Health_Group
  • https://www.mediawiki.org/wiki/Code_Health_Group/projects/CodeReview
  • DK: What is the process to get other people's attention on my code? Dig up ownership and add a team?
  • TK: Ownership of a particular code bases seems to be a general issue
  • JRB: Came across https://www.mediawiki.org/wiki/Developers/Maintainers page; but not necessarily reflecting, so we pushed for the concept of Code Stewardship to define ownership more explicitly, then making sure we're responsive; early stage; https://www.mediawiki.org/wiki/Code_stewardship_reviews
  • KH: Product ownership, less support by WMF to review volunteer contributed patches?
  • DK: There's a workflow for posting bugs on phab, but there's no process on what happens when I post a soluton ( e.g. a patch ). Proposal of having an automatic job that suggests to add a Phabricator task to a patch without one in the commit message.
  • TK: A lot of patches are incoming that are small and just useful, they can be merged without a bigger process around it. The real problem ariese when a vol. dev. has a vision for a new feature. That's again a problem of ownership.
  • JR: ownership and resourcing. Easier ones get done a lot quicker?
  • Tgr: PM dont have the expectation to be responsible for reviews of features appearing in gerrit; Have SLAs (service level agreements) per team (until when a patch should get a review)?
  • AK: For the records, some teams have SLA like https://www.mediawiki.org/wiki/Wikimedia_Language_engineering/Code_review_statement_of_intent and https://www.mediawiki.org/wiki/Reading/Component_responsibility ; Comments on task on phab to volunters that want to get something done, "This is in maintainence mode." Comments like that are not helpful and confusing because "that means it is maintained"
  • DK: New patches in Gerrit not visible/not seen by teams/maintainers? Reserve time in planning for helping out other teams/volunteers? Would like to see better use of +1
  • JRB: How do you feel about readiness (vs Work In Progress/WIP) of patches when reviewing?
  • Tgr: Review problem also within WMF, most patches are good. Three levels of review (right intention; architecture etc; then smaller things; see http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ ), usually first two can be performed already when the patch is still WIP
  • DK: Missing tests is the biggest problems. Do patches get a -1 when they don't have tests?
  • Fisch: Requiring tests set bar higher for volunteers, might need to help new volunteers
  • Tgr: Looking at which people reviewed lines of code before; lines affected by your patch; or git blame; could be automated?
  • Fisch: Writing on my own could be quicker than revieweing someone else's work; not enough time to explain everything in detail; downside that you don't help people to grow
  • LM: For WMDE the visibility is a lesser problem, because the code base we operate on is smaller, more isolated, number of volunteers is also smaller
  • LM: Why doing CR? - One side: Communicaton around CR is part of the learning process, and knowledge sharing. Time invested pays off inside of the team ( so also for volunteers ) - CR is not only for catching bugs, but also to share the knowledge.
  • KH: More like "these are the problems" than "here's what great"
  • ZF: -1 can be demotivating hence just leaving a commenting.
  • DK: There is a problem to fix via -1 allows filtering on workboard. There are only CodeReview and Verification labels currently in Gerrit, there could be more.
  • DK: pahb style tokens would be nice (love, kitten, cactus, ice cream...)
  • Fisch: -1 implies problem, could be orange dot or something. Or tokens/badges/barnstars for appreciation.
  • Tgr: -1 for a verification failure as it needs a manual rebase after ignoring a patch for a month is bad
  • Tgr: claims Gerrit is not getting enough attention with regards to maintenance. GG mentions Gerrit is regularly updated, there is no active functionality development
  • Tgr: It's hard to figure whats wrong on automated tests. You have to understand the comment of the bot, click the link, figure out what to do on that page, scroll a lot to find the error.
  • ZF: Gerrit update to 2.16 to happen, dates not scheduled yet
  • AK: Documentation of the Gerrit UI (Polygerrit) would need updates!
  • DK: We need advanced tool for power users on Gerrit, but the workflow is way better to me than the Github's one
  • ZF: +1 to having icons, or tokens to make comments or "-1" more "positive"
  • MG: I like that on Github you can suggest changes on the Pull Request, there is no similar functionality on Gerrit

Note: can be done on Gerrit, but is hard, and also not possible (?| on the level of Gerrit UI
Inline editing functionality will come back in Gerrit 2.16

  • TK: Expectation that everyone can change and add any code - it might be disappointing for new coming people when they submit contributions and they got rejected, or even more likely in MW case - rejected
  • Tgr: Suggests an excercise: Go to Wikimedia github project page, and count number of steps one needs to take to actually contribute
  • JRB: Another Q looking for feedback: Everybody in a team to review code; have a rotation; or even having a separate team?
  • Fisch: Separate team would have to know everything about everything, having a dedicated ( e.g. weekly ) slot, to sit down together as team and get some reviews done would work best for me personaly
  • Tgr: team values should be connected somehow to the values of the open source community, this could help include the code review in the team work cycle
  • KH: required are allocating time and having buy-in from the product management

Post discussion notes:

There exists a Gerrit Plugin for Intellij IDEs that sounds useful, but it doesn't seem to work?

I use the gerrit plugin with IntelliJ without issue, you need to set the URL to https://gerrit.wikimedia.org/r for it to work.