2020-03-03 Cross-Team PR Reviews Discussion

Owned by Craig McNally
Last updated: Mar 03, 2020
4 min read

Date

Attendees

Goals

  • Devise a plan to address the PR review bottlenecks created when multiple teams contribute to the same codebase
  • See slack for additional details

Discussion items

TimeItemWhoNotes
15 minReview ideas posted in slackCraig
40 minOpen DiscussionAll 
  • There's definitely a need to be clear about who is the tech lead for each team, who knows which codebase
    • tech leads are allocated to teams - publish a list on the wiki and keep it up to date.  FE/BE Leads for each team
    • code owners are allocated to repositories
  • Some form of delegation must happen
  • We need to be more proactive than reactive - don't wait until the PR is created/approved/reviewed,
    • Get buy-in from the various tech leads for a given codebase before the PR is created or even before the code is written
  • Rely on tools (PR check lists, sonarqube, raml-cop, etc.) to catch the no-brainers - test coverage, linting, etc.
  • Shore-up policies in places like PR check-lists, e.g. "are you adding new dependencies?  check with the other tech leads"
  • If the rules are broken or ignored, the tech leads need to discuss/sort it out.
  • Expand participation in the tech leads meeting?  Create a tech leads retrospective?
    • Need to be diplomatic about raising issues - not naming names/pointing fingers
5 minNext steps / Action itemsCraigWe're getting closer and agree on high level concepts, but need to get something concrete in place

Action items

  • Craig McNally Transcribe notes from slack (private channel) to wiki (here?)
  • Craig McNally Call a follow-on meeting to sort out the details - tomorrow's tech leads meeting?

Background/Notes

The following was originally posted to the #cross-team-pr-reviews slack channel, but since that's a private channel I've transcribed it here for greater visibility.

Overview

Quality reviews require experience, domain knowledge, and time. In a cross-team situations like core-functional, contributors don't necessarily have all three and therefore reviews tend to fall to a handful of people.

Defining code owner lists seemed to help a bit, but since then many of those code owners have left the project. Maybe refresh these lists, though there's ramp up associated with that.

This is still an issue, though lessened a bit by the scaling back of capacity on the affected team(s) (edited) 

Ideas

  • Have someone allocate PRs for review. 
    • Whomever does this would need to be informed about each person's time, experience and domain knowledge. They would also need to track PRs throughout their lifecycle.
    • This is not likely to be very popular
    • It seems to me that the time spent on this is better used simply helping review PRs.
    • (CAM) IMO this option should be off the table
  • Rotation of PR review responsibilities among code owners
    • The code owner lists need to be refreshed (should probably happen anyway)
    • One or two volunteers to help with PR reviews each sprint? The challenge here is that this is spread across multiple teams - when/how does this coordination happen? Maybe it's up to the teams to designate a single person each sprint to help with PR reviews (for teams that are contributing to a given domain, e.g. circulation?).
    • (CAM) Given the cross-team aspect of this I'm not sure how feasible this will be.
  • Restructure/Rebalance teams to avoid the cross-team aspect of this.
    • Do this periodically (every 3mo? 6mo?) based on planned work.
    • Would allow the problem to be handled within the team like other teams do (e.g. acquisitions). 
    • Example: Acquisitions has two volunteers each sprint to help the tech lead review/verify stories before they're closed. 
    • Managing something like this is much easier if it's contained to a single team - e.g. can be decided at sprint planning and evaluated during retrospective for effectiveness
    • This may require a greater level of communication between teams – maybe joint grooming sessions for features that span between the two teams… or maybe it’s something like ERM/Acquisitions/Inventory have done with the app-interaction group, which meets regularly (mostly at the SIG/PO/tech lead level and less so at the developer level).
    • (CAM) After speaking with Mark V., this is very unlikely to happen
  • Provide additional capacity where needed.
    • Allocate an additional "dev lead" for core functional that would help the tech lead w/ PR reviews
    • (CAM) Not sure how likely this is, or how it scales.
  • The tech lead from each team is responsible for reviewing PRs created by their team, regardless of the repository.
    • This can be delegated to a backend/frontend lead, but the responsibility ultimately lies with the tech lead.
    • In order for this to work and for tech leads to trust one another, it needs to be clear what goes into a quality PR review. PR checklists need to be rolled out and used. Additional documentation may be needed … What to look for, Level of detail, tips/tricks, etc.
    • Publish a list of teams and their structure and areas they work in and keep it up to date! Zak commented that he doesn’t necessarily know who’s working in a particular area/repository
    • Tech leads must have a succession plan. My understanding is that the EPAM teams already do this. When a tech lead leaves is replaced, their successor must understand what their responsiblities are, update the list above, etc.
    • I'm guessing there will be a learning curve in some places like circulation - maybe having the tech leads for teams that contribute to circ should "shadow" Marc for a PR or two - maybe do the review together on a call or something? IDK just a thought.
    • (CAM) I personally think this is where we should go with this... make it policy, make it clear to the tech leads that this is a new requirement, and educate all the tech leads as needed