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

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

Apr 22, 2020

Date

Mar 4, 2020

Attendees

  • @Craig McNally

  • @Zak_Burke

  • @Aliaksei Chumakou

  • @Alexander Kurash

  • @Vasily Gancharov

  • @Marc Johnson

  • @Richard Redweik

  • @Mikhail Fokanov

  • @Taras Spashchenko

  • @Martin Tran

  • @spampell

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

Time

Item

Who

Notes

Time

Item

Who

Notes

10 min

Recap

Craig

 

10 min 

Policies

All

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 min

Enablers

All

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: Documentation Changes - PR review and Tech lead responsibilitiesClosed