How to handle Pull Requests in a team of 10?

Question

My team consists of 10 devs. We develop some services that are closely connected to each other. We have our codebase on github.com.

Of course every change to our codebase must be reviewed before merging, so one always has to create a Pull Request (PR), wait for someone to review, maybe fix, and eventually merge.

So far so good.

BUT.

There are PRs that wait 1-2 days for being reviewed. And it's not about their length or complexity - it happens also when changes are small and easy. Do you want to merge your changes quickly, so you can continue with development? Well, ping the team few times, someone will take a look. You don't just create a PR. There must be a ping.

Why is it like this?

There are two problems:

1. You don't see automatically if there's a new PR waiting. As I said, there must be a ping. We don't want to rely on github mails (they are poor and we hardly use email in our organisation).
2. With so many people in the team, there are not strict rules of who should check your PR. So the people don't feel the urgency and pressure to help you, because there are so many other devs who can help. "Hey, team, help", you scream, and there's no particular person to help you.

So what we've done to make our lives easier?

1. For the first problem we set up a Slack channel. Every time you create a PR you post it on the channel, the whole team sees it and you wait till someone grabs it.
2. For the second problem we don't have a solution. You just notify @everyone at the channel, if you've been waiting too long or you're in a hurry.

However this creates another problem:

1. During a review there might be comments/questions/discussions. If you want someone to respond, you must notify him on Slack.

Now image one post on Slack with link to a PR with useless comments like "I commented on github, please check" or "I responded to your responds to my responds, please check".

And every time you surf between Slack and github...

So the questions are: How should we notify ourselves about new PRs and comments/fixes in already created ones? How can we avoid long waiting times for review?

Answer 1

This is a workflow issue with missing or ill-defined roles and task assignments against those roles. You are too focused on the technical solution and you have not concentrated on the flow of work that solution may or may not be able to provide. Put the tech aside and map the work from the initiation of a PR through its final stage. If you take the time to map out the work, you will easily identify the missing roles and what each role should be doing, and against which target performance metric.

Answer 2

Slack is a wonderful choice for communication. The problem I saw in your detailed explanation is that there isn't a primary person responsible for filtering PRs. There are a lot of developers and I think 1 or 2 persons should be charged with making sure PR that comes in reviewed by whomever needs to do that.

Also, the reason for "@channel" on slack is to notify everyone on a channel but it doesn't mean everyone would answer to mention which is why the cause of your third problem. I also suggest adopting agile methodology. This will help with the process you don't currently have and also help the team collaborate better.

You might also need to add a Project Manager to your team for these reasons:

1. Establish a single point of communication and accountability
2. Project management is your assurance policy
3. Ensure the project is organized to achieve the project goals
4. Cheaper to invest in the fundamentals now than later

Answer 3

Of course every change to our codebase must be reviewed before merging, so one always has to create a Pull Request (PR), wait for someone to review, maybe fix, and eventually merge.

One thing to ask yourself would be why the code review must happen before the merge. It doesn't have to be that way.

One alternate approach would be to institute automated checks - you can check for code style, test coverage, and static security and performance analysis with automated tools. If the automated checks pass, you can merge the change and defer the human code review for later.

If your main concern is getting changes integrated and available to the team faster, this would help address that concern. However, it may not address problems with understanding what needs to be reviewed and making sure those reviews happen.

You don't see automatically if there's a new PR waiting. As I said, there must be a ping. We don't want to rely on github mails (they are poor and we hardly use email in our organization).

This feels like an XY problem. It's not clear why a human needs to send a ping.

Email notifications are only one type of notification from GitHub. I would want to dig deeper into "we hardly use email in our organization" and understand why that is. I've found email notifications to be a great way for individuals to get information out of tools and onto todo lists and calendars.

However, GitHub itself provides other options. There is also a notification inbox. Outside of GitHub, there are integrations with messaging tools like Slack and Teams. You can have messages automatically posted to appropriate channels when actions happen in GitHub. This can get information out of various tools into places where your team communicates and coordinates with each other.

With so many people in the team, there are not strict rules of who should check your PR. So the people don't feel the urgency and pressure to help you, because there are so many other devs who can help. "Hey, team, help", you scream, and there's no particular person to help you.

When everyone is responsible, no one is responsible.

One option would be to set up a CODEOWNERS file to automatically add people to a code review based on where the changes are. For a more manual approach, you can look at the git history of a file and see who the previous authors are, since they may be in a good position to review the code. There is some work to automatically infer reviewers from git history, but I'm not sure how effective these automatic techniques are.

So the questions are: How should we notify ourselves about new PRs and comments/fixes in already created ones? How can we avoid long waiting times for review?

It seems like the integration between Slack and GitHub would go a long way to start, since you can get notifications about new issues, pull requests, and comments right in the communication tool of your choice. But overall thinking through your process and what you hope to achieve is the true solution.

Answer 4

"Of Course" Your Merge Process is Out of Scope for Project Management

My team consists of 10 devs. We develop some services that are closely connected to each other. We have our codebase on github.com.

Of course every change to our codebase must be reviewed before merging, so one always has to create a Pull Request (PR), wait for someone to review, maybe fix, and eventually merge.

Anytime someone says "of course" the first thing you should do is reconsider your initial assumptions. Otherwise, you're creating X/Y problems and tautologies that will lead you astray. Instead, you should be empowering your development team to reduce friction in the process without reducing quality in whatever ways make sense to them.

Code Merging is a Technical Concern, Not a Project Management Practice

Let's start by addressing the elephant in the room: how a development team develops is a development team concern, not a project management concern. From a project mangement perspective, the only concern should be whether the team is making progress at a predictable cadence towards a potentially-deliverable product increment within defined measures of quality.

In this case, you're making an ex nihilo assumption that this extremely manual and sequential process is intrinsically necessary. However, you've done so without defining:

1. The value of a manual code review.
2. Why your tooling is driving your process, instead of selecting and configuring tools that support a low-friction process.
3. Why you have a low-automation process in the first place.
4. Why continuous merging to a shared development branch doesn't provide a Pareto Principle solution to your perceived process problems.

In short, project management should be concerned with outcomes related to schedule, budget, scope, and (arguably) quality while leaving the technical implementation details to the people implementing them. If you're a project manager, that's not you; if you're a developer, then you should be talking with the rest of your development team about the things that are causing you pain.

Define Your Process, Then Automate It

Code reviews can serve many purposes, but code quality is rarely one of them. In general, you should have a Definition of Done that includes things like:

1. Writing your tests first, so that you have a clear Definition of Done for each feature.
2. Clean patch sets and feature toggles so that changes can be easily reverted if necessary.
3. Code that conforms to the living document that is the team's current style guide.
4. Code that passes automated linting, unit, functional, and integrations tests that act as executable documentation.
5. Automated quality gates that prevent regressions and predictable classes of bugs.

How the team does these things is not the concern of project management, but making sure that the team is empowered to figure out how to do these things in a way that's optimized for the team and the project is certainly within the scope of project management. However, the best way to fail at this is to mandate sequential processes that ultimately boil down to the "million monkeys" fallacy.

The idea behind most of the code reviews of the type you describe is the same as the notion that given infinite time and a sufficient number of monkeys, random typing will reproduce the works of William Shakespeare. While mathematically true, it's a ridiculous way to manage product quality. More random eyeballs on the code don't make the code better, nor does arguing about whether some piece of code should use a case statement or a ternary operator. Almost everything about a code review can and should be automated except validating readability and knowledge sharing. Unless that's what you're doing with your code reviews, whatever you're validating during "code reviews" is going to be subjective, error-prone, opinionated, and fallible.

CYA Isn't a Substitute for Quality Gates

The kind of process you're describing isn't designed to test code quality or improve team communications. Its sole purpose is to spread responsibility in a way that ensures no one is really responsible.

Software bugs are inevitable. Your current process seems primarily designed to ensure no individual person is blamed for the bugs, rather than actually preventing them. The real solution is to improve your process to actively catch as many bugs in an automated and repeatable way as possible.

Test-first development processes, automated testing, continuous integration, feature toggles, and any number of other modern software development practices can help with that. Trying to smooth the communications flow of a fundamentally flawed process is solving the wrong problem. If you choose to focus on that to the exclusion of addressing the underlying process issues, you get to keep both halves of the broken process. Q.E.D.