2020-03-03 Cross-Team PR Reviews Discussion

Date

Mar 3, 2020

Attendees

@Craig McNally
@Zak_Burke
@Marc Johnson
@Marko Knepper
@Aliaksei Chumakou
@spampell

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

Time	Item	Who	Notes

Time	Item	Who	Notes
15 min	Review ideas posted in slack	Craig
40 min	Open Discussion	All	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 min	Next steps / Action items	Craig	We'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