2020-03-04 Cross-team PR Review Discussion Part 2

Owned by Craig McNally
Last updated: Apr 22, 2020 by Mike Gorrell

Date

Attendees

Goals

  • Come up with a list of explicit next steps and/or action items

Background

From previous meeting: (2020-03-03 Cross-Team PR Reviews Discussion)

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 responsibilities 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.
  • 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

Discussion items

TimeItemWhoNotes
10 minRecapCraig
10 min PoliciesAll

Things the TC will officially mandate:

  • The tech lead from each team is responsible for reviewing PRs created by their team, regardless of the repository.
    • May be delegated to a backend/frontend lead, but the responsibility ultimately lies with the tech lead.
    • The lead maintainer for each repository is still responsible for their designated repositories.
  • Tech leads must have a succession plan with sufficient overlap for knowledge transfer.
    • Onboarding/handoff must ensure that the new tech lead understands their responsibilities and duties.
    • Updating team documentation (who is the tech lead(s)) is the responsibility of the Tech lead.
  • Issues discovered after a PR is merged will be addressed by the tech leads involved during retrospective, out-of-band discussions, etc.
  • Feature-level design must be captured on the wiki for purpose of early feedback / knowledge sharing among tech leads
40 minEnablersAll

Process improvements, meetings, tooling, documentation, etc. that will help implement the new policies:

  • Tech leads go back to their teams and promote PR reviews by all devs - stress the benefits.  
  • Publish a list of teams and their structure and areas they work in.  Once created this will be maintained by the tech leads.
  • Institute regular tech leads retrospectives (each sprint? every other sprint?) and re-engage ALL tech leads
  • <something about getting the various tech leads involved in design/arch conversations earlier>
    • Promote use of wiki for capturing designs / spikes / etc. ?
    • Decision logs - part of the documentation conversation.
  • Group review of a PR led by Marc? Zak? - if not live maybe recorded and shared with appropriate tech leads?
    • May not be needed by the retrospective / design feedback loop / etc.

Action items

  • (All) Tech leads go back to their teams and promote PR reviews by all devs - stress the benefits.  
  • Craig McNally Publish a list of teams and their structure and areas they work in.  Marc Johnson and Taras Spashchenko to provide links.
  • Craig McNally Jakub Skoczen Organize tech leads retrospectives - initially every sprint, eventually space out to every Nth sprint
  • Craig McNally Reach out to all current tech leads / call a meeting dedicated to going over this - what are their new responsibilities, promoted practices, etc.
  • (TC) Define documentation policies/formats/templates - farmed out to a subgroup, etc...
  • Craig McNally polish the policy wording and get to David Crossley to put on dev.folio.org →  FOLIO-2506 - Getting issue details... STATUS