Congratulations! This is one of the 52 proposals that made it through the first deadline of the Wikimedia-Developer-Summit-2016 selection process. Please pay attention to the next one: > By 6 Nov 2015, all Summit proposals must have active discussions and a Summit plan documented in the description. Proposals not reaching this critical mass can continue at their own path out of the Summit.

This task is still missing clear Summit goals, topics that could be discussed here before, and active discussion. It would be good to sort out these problems before the next deadline on November 6.

A lot of discussion has happened in T114419. What we intend to do before the summit is to publish a draft code review RFC, then refine it during the session at the dev summit.

I created a repo for code review documentation: rRFCCR Code Review RFC

Per our conversation on IRC/Freenode at #wikimedia-releng, we can expect to see the first draft of the RFC filed later today. The first draft doesn't need to be a thing of beauty; it just needs to be the canonical placeholder. @mmodell suggested that this may possibly be T114311 might be the right placeholder, but I haven't had the chance to clarify with him.

(1) from the description is handled, RfC filed: T119908: [RfC]: Migrate code review / management from Gerrit to Phabricator. T114311 is more our team tracking of the things we needed to accomplish this quarter.

This session currently isn't in consideration in the T119032: WikiDev 16 working area: Software engineering area. @mmodell, @demon: does that seem correct to you?

For what is worth, is one of the only two must-haves defined at T119030: WikiDev 16 working area: Collaboration.

@Qgil: I suppose that's true (if I'm reading T119030: WikiDev 16 working area: Collaboration correctly), but you're currently proposing ceding the Collaboration spot on the Robertson 1 schedule exclusively for T114419: Event on "Make code review not suck". Perhaps we can have T114419 and T114320: Code-review migration to Differential status/discussion in the same session? If not, what Robertson 1 topic do you suggest that people can miss in order to participate in a discussion about T114320 (Differential).

T114320 (Differential) is potentially a pretty good partial answer to the question "how do we simultaneously make our software more logical and obvious for all contributors, and make it more useful and reliable for the Wikimedia sites?", so it should be considered when constructing the priorities for T119032: WikiDev 16 working area: Software engineering. That is especially true given my comment at T119032#1846945 where I tried to clarify the scope and minimize overlap of T119032 (Software Engineering) and T119030 (Collaboration).

I would hate to let other proposals in the Software Engineering area (T119032) escape comparative scrutiny with T114320 (Differential). That said, T114419 (code review) and T114320 (Differential) is a pretty natural pairing, and putting them in the same 70-80 minute block would ease our scheduling woes a little, and probably make life easier on @daniel. Your call.

Both code review sessions are obviously related, but each topic is big enough to fill its own slot.

Now that the Robertson 1 slots are broadly defined, I would simply let the owners of this presentation decide their preferred slot in Robertson 2 or 3. While most Summit attendees will use code review, not that many will be as interested in the discussion about the migration as to miss a session about X. If RelEng has a sense of who should be there, then avoiding direct conflicts will be simpler.

I don't really care which track this ends up in. I figure it'll have a huge overlap with T114419: Event on "Make code review not suck" anyway.

There's far too many tasks re: dev summit for me to keep track of so I'm not even trying anymore :p

I have proposed the session for Monday at 10am: https://www.mediawiki.org/wiki/Wikimedia_Developer_Summit_2016#Monday.2C_January_4

If you find a better slot, please go ahead and move it.

@ori's proposal is fine by me. In fact, that's pretty much how Differential works by default if we assume that everyone who currently has review rights keeps their merge rights in Phab-land. Code review in Phab is a social construct, not a requirement. In fact, it's actually fairly difficult to lock down Git in the batshit insane ways Gerrit does.

FWIW, we can easily add herald rules so things that aren't reviewed first can be flagged in Audit (a-la how we used to use Extension:CodeReview for post-commit review).

In T114320#1904170, @demon wrote:

@ori's proposal is fine by me. In fact, that's pretty much how Differential works by default if we assume that everyone who currently has review rights keeps their merge rights in Phab-land. Code review in Phab is a social construct, not a requirement. In fact, it's actually fairly difficult to lock down Git in the batshit insane ways Gerrit does.

FWIW, we can easily add herald rules so things that aren't reviewed first can be flagged in Audit (a-la how we used to use Extension:CodeReview for post-commit review).

+1 to everything @demon said.

I also do not see any problem with @ori's proposal.

In fact, it's actually fairly difficult to lock down Git in the batshit insane ways Gerrit does.

Can you back that up with technical facts? The update hook in a Git repo is just an executable run with specific arguments whose exit code is checked to determine if a push will be accepted, so it can just check what a database says about the review status. But I have seen nobody suggesting that we should do that.

Code review in Phab is a social construct, not a requirement.

That is also how our gerrit is configured. AFAIK, nobody argued for doing this different in Phab. The counter proposal seems to be for it to remain a social requirement and fix the reviewing instead.

However as pointed out in the task where ori made that proposal, that proposal does not actually solve any of the problems pointed out with reviews (you need someone else to +2 your change if you don't have that permission). I'll further comment in T114419.

Etherpad for the Summit session: https://etherpad.wikimedia.org/p/gerrit_to_phab

Notes from session

Goal

Try to find workflow sticking points and create a shared understanding

Want to use Phabricator for

• Code hosting
• Code review
• Deployment

Topics

Somethings that will change

• Deployment pipelines less simple
• Strict merge approval
  • Code review in phabricator allows people in a group to push to upstream repo
  • Everyone will have merge access
  • Doesn't require pre- or post-review, but allows for both workflows

• Arcanist
  • Post-commit review workflow does not require arcanist
  • Branch workflow does not require special tooling
  • Upstream is working on allowing differential code-review without arcanist—not ready for prime time
  • Not very complicated
  • Self-updates (arc update)
  • PHP Dependency
  • Less bad than git-review
  • SHA1 changes on merge
    • Treats change as more of a patch
    • Squishes changes to one commit
    • Immutable mode preserves the full history
    • Person doing "land" can make the call whether or not to squish

• CI changes
  • .arclint - lints affected parts of code-base
  • .arcunit - runs unit tests on affected parts of code-base
  • phab encourages more local-testing
  • Add code-style checks
  • QUESTION Some people are unable to run unit-tests locally
  • Harbormaster could replace zuul
    • Zuul contains a large amount logic
    • Extensions and dependecies inside zuul
    • This is going be quite a bit of work
  • Early testing shown that zuul may not be necessary
    • Can see detailed output inside phabricator
  • Still use jenkins

• Chain of deps in a single repo
  • Able to mark changes as dependent on one another

• Cross repo deps
  • Needs testing, but cross-repo dependencies should be supported

• Linters
  • Shows up as having lint errors on the patch
  • example: pep8 python rules can be configured
  • Annotates what in your patch with unit-patch coverage
  • All repos will need an .arclint or .arcconfig file
    • Mostly boilerplate one-liner
    • No specific templates
  • PHP linters will shell out
  • Can write your own linters
    • We have a fork locally, but upstream is receptive to changes
  • RelEng can offer support

• Next steps
  • Mediawiki and extensions/Ops-puppet is a very large change, not ideal to move at this time
  • Look for standalone projects to move
  • Critical mass of users is important
  • Operations .deb's are a good use-case

• How to migrate to differential
  • Put an .arcconfig/.arclint/.arcunit file in the repo, merge that
  • Close out back-log of changes
    • This is a convenience item
  • Mark the gerrit repo as read-only
  • Done!

• ACTION Migration of code-reviews from gerrit to differential
  • This is a tool that should be built
  • @mmodell to work on this

• repository-administrator group
  • Anybody in that group can add anybody to that group
  • a viral creation of repository

• ACTION Preserving gerrit info
  • Comment history in gerrit
  • Plus one, minus one etc
  • It is kept in refs/notes
  • Browsing those comments in phabricator, not possible likely
  • Static HTML dump/SQL dump of info from phab

• What does Mediawiki code-review look like?
  • a more open process
  • phabricator doesn't make you merge everything you approve
  • everyone who is reviewing is able to push
  • (Also a merge-button in phab)
  • Deployed branches would become separate repositories
  • Long-lived deployment branches rather than a fork

• How is the process different for the community contributors?
  • Community contributors can submit a diff
  • Phab code-review is more closely

• ACTION Guides for workflows
  • @mmodell wants to write these guides
  • As-git-as-possible workflow should be documented
  • There is a phabricator task for this
  • Should be written before any high-volume repos move
  • Gerrit-to-differential process

• Herald rules to flag commits that have merged without review "Cowboy commits"
  • Will poke folks within the project if anyone pushed past review

• "Owner's Tool"
  • Assign repos, and sub-sections of code to specific people
  • Those people will be notified
  • Can setup yourself via Herald

• Phabricator has no -1 +2
  • This is replaced by comments
  • You can "Reject" changes would be like a -2
  • "Accept"ing a change equivalent to a a +2, but without an auto-merge

• How will this play with github
  • Facebook does have a tool to grab pull requests
  • The projects that are currently on github at the foundation are because of CI
  • Would like to keep tools on differential/diffusion
  • We never want to deploy code from a third-party

• Updating others changes
  • Cannot edit the commit from the browser
  • The commit message could be modified in the browser
  • Amend others changes as part of "land"ing a patch
  • Will update the diff with the changes you made
  • These minor changes would trigger unit tests
  • Would be considered code-review

• Design research team could gather workflows
  • There is an open task for that T117085

• Timeline
  • It will take longer than we think it will
  • There will be a long-tail
  • Critical repositories: 6 months seems reasonable
    • There will be repos that can't move in that timeline
    • There will be a long-tail
  • Will need one-off projects to move
  • There is no hard timeline, waiting for RFC to firm
  • There is time allocated within RelEng

In T114320#1904418, @JanZerebecki wrote:

In fact, it's actually fairly difficult to lock down Git in the batshit insane ways Gerrit does.

Can you back that up with technical facts? The update hook in a Git repo is just an executable run with specific arguments whose exit code is checked to determine if a push will be accepted, so it can just check what a database says about the review status. But I have seen nobody suggesting that we should do that.

Phabricator actually has a default commit hook that can check this specific thing.

Linking relevant tasks for the ACTION items from the meeting:

In T114320#1914904, @thcipriani wrote:

• ACTION Migration of code-reviews from gerrit to differential
  • This is a tool that should be built
  • @mmodell to work on this

T122979

• ACTION Preserving gerrit info
  • Comment history in gerrit
  • Plus one, minus one etc
  • It is kept in refs/notes
  • Browsing those comments in phabricator, not possible likely
  • Static HTML dump/SQL dump of info from phab

T617

• ACTION Guides for workflows
  • @mmodell wants to write these guides
  • As-git-as-possible workflow should be documented
  • There is a phabricator task for this
  • Should be written before any high-volume repos move
  • Gerrit-to-differential process

T117058