A Pull Request Journey

PR Review WG

Stefano Magni - Rob Dominguez - Daniel Chambers

The problem:

Our PRs

stay open

for 7 days

Stats

Β 

10 interviews

6 meetings

3 documents

Slides are boring!!

Storytelling, please!!!!

The story is fiction 😊

Β 

But the problems

are real 😊

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ’‘

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ’‘

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ’‘

1 minute later...

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

I know how

to help you...

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

I know how

to help you...

CODE OWNERS!

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

CODE OWNERS!

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

CODE OWNERS!

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

CODE OWNERS!

1 day later...

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

CODE OWNERS!

1 day later...

2 days later...

❓

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

CODE OWNERS!

1 day later...

2 days later...

3 days later...

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

CODE OWNERS!

1 day later...

2 days later...

3 days later...

πŸ’‘

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

CODE OWNERS!

1 day later...

2 days later...

3 days later...

πŸ’‘

New task!

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

CODE OWNERS!

1 day later...

2 days later...

3 days later...

New task!

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

1 day later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

1 day later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

1 day later...

1 day later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

1 day later...

1 day later...

1 day later...

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

1 day later...

1 day later...

1 day later...

1 day later...

πŸ‘¨β€πŸ’»

1 minute later...

2 days later...

2 days later...

1 day later...

1 day later...

1 day later...

1 day later...

πŸ‘¨β€πŸ’»

1 minute later...

1 week later...

πŸ‘¨β€πŸ’»

1 minute later...

1 week later...

1 month later...

1 minute later...

Let's recap the problems!

1 minute later...

Let's recap the problems!

🚨 No PR description 🚨

1 minute later...

Let's recap the problems!

🚨 No PR description 🚨

🚨 Big PR 🚨

1 minute later...

Let's recap the problems!

🚨 No PR description 🚨

🚨 Big PR 🚨

🚨 No direct reviewers 🚨

🚨 Notification missing 🚨

🚨 Reviewers have no time 🚨

🚨 Timezone incompatibility 🚨

🚨 CI issues 🚨

How

to fix them?

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ’‘

1 minute later...

🚨 No PR description 🚨

βœ… Describe your PR in details βœ…

βœ… Write tests βœ…

🚨 No PR description 🚨

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

2 days later...

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

CODE OWNERS!

🚨 Notification missing 🚨

βœ… Assign the PR to individuals βœ…

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

🚨 Notification missing 🚨

βœ…Β GH Slack integration + Haystack report βœ…

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

🚨 Big PR 🚨

βœ… Reduce PR size βœ…

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

πŸ‘©β€πŸ’»

πŸ‘¨β€πŸ’»

1 minute later...

🚨 Reviewers have no time 🚨

βœ…Β Explicitly allocate time for reviews βœ…

πŸ‘¨β€πŸ’»

πŸ‘¨β€πŸ’»

2 days later...

🚨 Timezone incompatibility 🚨

βœ…Β Assign PRs to same-team peers βœ…

πŸ‘¨β€πŸ’»

1 day later...

1 day later...

1 day later...

1 day later...

🚨 CI issues 🚨

πŸŽ‰ There's a dedicated WG πŸŽ‰

PR authors must

  • Reduce the scope of the PRs
    Β 
  • Describe PRs in details
    Β 
  • Write tests
    Β 
  • Assign PRs to individuals from their own team
    Β 
  • Do not start working on new tasks until the PR is merged
    Β 
  • Use Github's Slack integration

PR reviewers must
Β 

  • Dedicate some time slots every day to review other people's PRs (the cross-team ones too)
    Β 
  • Report the tested functionalities
    Β 
  • Use Github's Slack integration

Team managers must
Β 

  • Understand that PR reviews are a team's responsibility, not just an individual's responsibility
    Β 
  • Retrospectively understand if/why people need a lot of time to review PRs
    Β 
  • Enable Slack reports using Haystack for the team PRs

Guess what?
A document with these best practices already exists!

What happens if we do not apply

these best practices?

That we start working on a lot of things

but we release just a few...

A lot of bugs hit our users

That a lot of PRs remain orphans and

we need to automatically close them

What about the ideal situation?

More pair programming

No blocking reviews but post-mortem reviews

A lot of trusted and stable tests

Fast and reliable CI pipelines

PRs are released in production right after being merged

What's next?

βœ… Include PR reviews

in the onboarding docs

⏳ Check how long the PRs

stay open on a monthly basis

⏳ Spread the word and the resources

WE NEED YOU!

PR reviews cannot be improved

without the individual efforts

of each PR author and reviewer!

Thank you!

PR Review WG

Stefano Magni - Rob Dominguez - Daniel Chambers

For getting interviewed
Nikunj - Abby - Tom Harding - Nithin - Priya

Jesse Hallett - Solomon - Shraddha

Daniele - Karthikeyan

For participating

Aaron Johnson - Shraddha Agrawal - Auke - Bilal - Brandon Martin - Brandon Simmons - Daniel Chambers - Daniel Harvey - Greg - Aravind KP - Manas - Marion - Matthew - Nicolas Beaussart Hatchuel - Ojas - Rikin - Samir - Solomon - Stef - Vamshi - Vijay

PR authors

  • Reduce the scope of the PRs
  • Describe PRs in details
  • Write tests
  • Assign PRs to individuals
  • Do not start working on new tasks

PR reviewers

  • Dedicate some time slots
  • Report the tested functionalities
  • Use Github's Slack integration

Team managers

  • Get the PR to review a team's matter
  • Enable Haystack reports for PRs