That’s the question I asked my colleagues and the result was, well … not so surprising.
Many teams have a chat channel where they post the link to their merge request.
If someone does a review for a MR, they add an eyes 👀 emoji to the message, if it’s approved they add a checkmark ✅ and if there are questions they add a speech bubble 💬 or cross mark ❌ .
Other teams only assign people directly in GitHub or GitLab and the ticket system.
And some do a mix of both of them (like my team). If nobody reacts to a MR in our chat, we pick one 🤗
What is interesting: In some teams the reviewer merges the MR, in others the person who wrote the code.
Pressing the merge button is a success experience and we wanted to leave that to the person who wrote the code
But you could also say: Who merges the code is responsible for breaking the pipeline 😅
So how are you doing it?
Message in a channel? Assigning your Team Lead? Or something completely different (drawing straws)? Let me know in the comments.
Top comments (7)
I personally prefer to share open reviews to a group of people than assigning it to a person directly. And I'm in camp "merge by the person which wrote the code" because I do believe it is a success moment.
There is more that could be part of the process e.g. the description of the review (providing screenshots, links to the tickets, etc) and also the size, the review should be as small as possible.
The last thing I want to mention is based on a comment from @kotzendekrabbe : whatever your process is you should document it; make sure the whole team can look it up at any time.
The agency I work for has built up a strong code review process over the years.
We use GitLab to create a merge request for code changes. We have pipelines set up that when an MR is created runs the code through coding standards tests. For projects that have unit tests, these are also run. If the pipelines pass then a temporary version of the site being worked on is spun up on a Continuous Deployment server. This allows reviewers to be able to test any new functionality, the MR author is expected to provide some test instructions in the MR description.
Each new MR is assigned to two developers. We're a team of about 15 developers, who are then split into 3 sub-teams. Each sub-team is responsible for particular clients and projects. When we assign reviewers we select one from the associated sub-team and one from the wider development team. We've found this helps a lot as one of the developers reviewing the code will have some familiarity with the project, whilst ensuring we still make use of the experiences from the wider team.
Assigning reviewers is all handled by an internal tool we've developed. It also ensures that we only assign people who are not off-work.
The two reviewers will test out the new functionality and if happy, review the code. Feedback is welcomed, whether that's highlighting bugs, suggestions or just asking questions. The code review process helps us catch issues before they go further down the chain, and educate the wider team by looking at how others approach problems.
Once the reviewers are happy they thumbs up the code. The second reviewer to do this hits 'merge'. Our pipelines then deploy the code to a staging server and update Asana where we manage our tasks.
Every other Friday, we get to work on non-client projects. We've used this time to really develop our internal tools for the code review process. We have a Chrome/Firefox plugin that alerts people to merge requests that need their attention (either they need to review it or respond to feedback). We also have a daily notification email that summarises what merge requests need the recipients attention.
It's interesting to read how others are handling this process as we're always open to updating our processes.
Cross-Team reviews are a challenge I also encountered. Often the other person didn't feel "fit enough" to review the code. Having another reviewer from your own team encourages the "external" reviewer and makes them more comfortable.
Question out of curiosity: How far is your tool automated? Does it assign people automatically or does it just suggest some, but you still pick by hand?
It automatically assigns reviewers, although we’re open to others taking a look. We found that assigning people helped encourage those less confident in reviewing to get involved. We felt this was important as reviews aren’t just about checking the code, they’re also a learning experience.
This is the process used at a previous job, not my current one. However I believe it was one of the best I have participated in, helped keep the quality of the code high, and created some great discussions and learning experiences for developers.
Once a story/feature/whatever is ready the developer working on the story creates the PR. They include reviewers from across teams, not necessarily across disciplines however. This may not be possible everywhere if the teams are structured differently, but basically getting 3-4 people on a PR was the goal.
The PR was announced in our chat, tagging the people who were requested to review. And generally a timeframe was given like, "Hey everyone, looking for feedback on story #1234. Hoping for final feedback by EoD Wednesday." Everyone then gives their feedback, and the developer responds with a change based on the comment, or a discussion why they won't/can't do things differently. If it's a quick conversation that would happen in the PR comments. Sometimes bigger discussions took place, but the PR comment was always addressed in one way or another.
Once everyone is satisfied their comments have been addressed, they sign off on the PR, and the developer is then ready to move along in the process to merge the story. The developer that completed the story is the one who ultimately did the merge.
For the most part the process worked really well. It can be a bit daunting at first, but I definitely learned a lot from those reviews. They're much more streamlined than a traditional, everyone in a room, go through code for stories, and review kind of process. They fit into the day much better as well.
Giving a timeframe for the review is an excellent idea I will adapt right away!
We use trello to manage our workflow so a card gets moved to the review column and a dev is picked that is most suited to the review or most available. And if someone outside of dev needs to review the feature then we move the card to external review and ping the appropriate person on the card.
To merge the PR usually the person who made it merges it but sometimes another dev will if we need to deploy.