Pull request process
Posted by arlitsa@reddit | ExperiencedDevs | View on Reddit | 15 comments
Say you have a large team, 20-40 people. Many sister teams want to plug into your functionality. Seniors are cross cutting and are freely able to review code for sets of overlapping projects. Various devs may not be experienced with all tool chains.
How to prevent PR starvation ?
Problem:
- One senior dev might be a silo of experience and context on a project.
- Sister teams might have their PRs ignored if the main senior is bogged down with something.
- Juniors might be afraid to review something they’re not confident in. May also rubber stamp something and let low quality code into master. *Ignored PRs might get escalated to management if they languish a while.
We currently follow a work request stealing model where people post everything in one channel and most things get reviewed. Sometimes sister teams just tag a set of seniors asking for review so it’s a lot of seniors reviewing code.
PickleLips64151@reddit
I got tired of always being the one called on to review code where I had deep domain knowledge.
So I dragged my two juniors into the process.
I did one, where I talked it through and they watched. We did one together, where I would ask questions to help them get started. They did one where I mostly watched. Now, they do them without my help.
If you don't mentor your juniors, you have zero credibility to complain about not having capable juniors.
MoveInteresting4334@reddit
Yes yes and yes. You show, you pair, you monitor, then you can trust.
_Invictuz@reddit
Good ol spmt?
Brilliant_Law2545@reddit
Finally some good advice on this Reddit.
arlitsa@reddit (OP)
One idea I had is maybe run a report and those infrequently reviewing e.g juniors, are asked to pair with a senior for an hour to figure out what sorts of things to look for on a PR.
Another idea is to set up tiers where first a junior reviews it and then sits down with a senior to re-review.
gemengelage@reddit
The tiered reviewers approach worked well for me in the past. I had a project where other teams would sometimes create PRs for my team's product.
We had a codeowners file to force the respective team to review certain parts of the codebase when they are being changed by a PR. The other teams don't really own the code, but there's some co-ownership and before that we had some issues with the teams stepping on each other's feet.
Then there's a first reviewer. If the author is on my team that's some teammate, they're from a different team, it's their tech lead. That first reviewer has to approve before I even look at it (unless specifically requested). This process can help a lot to spread knowledge, get juniors into reviewing and most importantly, reduce the amount of time I spent on that PR, because (at least in theory) all the obvious issues were already filtered out by the first reviewer.
Some teams also have an initial reviewer internally, so that pushes the number of reviewers to three. If that team also touches modules that are marked in codeowners, one PR might end up requiring approvals from five different people.
Yet it's somehow still quicker and more pleasant then our earlier model, where all PRs from multiple teams would pile up on a single person.
Omegaprocrastinator@reddit
As you saying pairing people and having dedicated review time with an emphasis on priority pieces of work would work best. Just observe and take notes on new paint point that will potentially rise
bwainfweeze@reddit
Siloes don’t exist. You only think it’s a silo because the moat is so goddamned deep.
The siloed person should have two devs training up to be bus number 2&3. And the py can start by taking the PRs seriously, even if they only start out as rules lawyers or asking questions the author can explain away.
PRs aren’t just about making the resultant code better. They’re about buyin on the code you end up with.
BitSorcerer@reddit
I wish my team was 20 devs deep, sounds fun.
ivancea@reddit
Pheww, some things here:
20-40 devs in a team? That's crazy. A tab of 10-15 if anyway considered large, 20-40 feels ridiculous. I suppose there are "sub-teams" formed within it. In which case, splitting the team would be the first step to correctly organize it. This is important, because you're suffering from a lack of organization, basically.
Juniors will always be afraid of that, yeah. But they should review them anyway. Just ensure that they learn from it, that you encourage them to review, and that a acknowledgeable dev approval is still required to merge. Eventually, those juniors will know their domain, and their reviews will be considered as "approving" for most things.
In general, reviews should be part of the work. If you don't have the tooling (like swarmia or something to help you), make sure everybody takes some time to review things every day. Without knowing the team better and the team issues, it's hard to say much more
dbxp@reddit
First I would split the team because I really don't want to be in a scrum with 40 people or schedule 121s for that many.
As for the problem at hand I would have seniors do code review with one of the mid levels joining on them on a call as a form of mentoring. In the past we did this with the person who has developed the code and they would walk the person through but I found it was difficult to delve into and investigate things whilst on a call however I think you could make it work. I would also look into doing some documentation activities, you can do this various ways ie assigning a pair of devs to a task and having one document it and the other write the code.
PurepointDog@reddit
What is "schedule 121"?
cach-v@reddit
One-to-ones 😂
Icecoldkilluh@reddit
We have a very similar process to yours and a similar problem.
My theory is that the effect of no one doing PR review is something like the bystander effect
I.e everyone is responsible for code review but no individual feels responsible
The solution is to assign people responsibility (either for whole apps or parts of the codebase if it’s a monolith). Then hold them to account for actually doing it/ reward them for doing it well
People are motivated by carrots and sticks. If there is no repercussion for not doing pr review, and no personal gain for doing it well - people will just avoid it
Inside_Dimension5308@reddit
Devs need to learn how to do code reviews. The entire process should provision that.
I as a lead/senior person might review a lot of code but I am not the first person to review. I ask junior developers to review the code first. Once they are done with their review, I start. Once I am done providing the review comments, I ask the junior developers to basically check and learn from my comments, ask queries related to comments. At the end, a PR becomes a learning tool. The target is juniors maximize defect detection so that seniors dont need to. This also builds trust over a period of time.