Skip to content

Gitlab MR Flow process

Introduction - Code review flow 2023

Even though some MR’s are going smoothly, they tend to stay unresolved for a long time, and once they are reviewed they tend to spawn quite a lot of changes that might take a long time. Right now the MR’s are quite hard to plan, and we should try to optimize that flow. This guide is an attempt on how this can be done. After implementation, we should revisit this and adjust it based on the findings.

It is to be noted, that a lot of the current problems with the MR flow, can be addressed to too many ongoing features. This flow change should be implemented alongside our current plan to reduce the number of features being worked on at the same time.

Key functionalities addressed in this change:

1. How do we ensure a faster MR flow – without compromising the quality?
Right now an MR can take quite a while from creation to finish. This can lead to merge conflicts with the master branch, and makes it tougher to create new features that might affect the same area in the code, or being dependent on the branch currently being reviewed.

Right now an MR tends to be forgotten when created, and if the reviewer is busy it can get stuck for a long time.

This guide should help combat this, and make it clear how we notify each other and how we make sure an MR isn’t stuck in the flow.

2. When does something entitle a change, and when is it considered “Nit picking”?
One of the problems right now, is that an MR can be a major undertaking. It can contain a lot of threads and take a long time to go through.

In many cases, a thread might be a valid issue to be tackled, but it might as often be the personal opinion of the reviewer as to if a comment should be removed, if something could be changed in the future etc. And while the assignee can choose to ignore the advice in the thread, it still creates an unnecessary long review process.

This problem is addressed by re-introducing “NIT” when writing a review (For “Nitpicking”). If a thread is only a comment, or “nice to have”, the text should be prefixed with “NIT”.


Code review flow 2023

Daily MR flow

  1. Check Gitlab daily, preferably in the beginning of the day.
  2. When you have created a MR, it’s still your responsibility to make sure it’s completed, and that a reviewer is assigned. If you haven't heard from the reviewer after a 24 hours, poke him/her.
  3. If you need to poke a colleague, ex.: If you have created an MR, finished a review or any other action that needs response from someone else. Send them a message directly on Slack.

Creating a MR

  1. An MR should be named using the following convention: If a branch is called “HS-1234-some_branch_name”, the MR should be named “HS-1234: Some branch name”.
  2. As a general rule an MR should always have a Jira task, and vice versa. An exception to this could be deployment bugs etc.
  3. In most cases only one reviewer should be chosen. If more than one are chosen, it should be mentioned when poking the reviewers, so they can coordinate the review. And if multiple are added in a situation where it can be either – the first to review should remove the other reviewer from the MR.
  4. If an MR isn’t ready for review, it should be marked as DRAFT. An MR can be marked as DRAFT for different reasons, but if the code base has changed so much that it can be considered a new task, a new MR should be made instead.

Reviewing a MR

  1. A review should be short and to the point, and discussions shouldn’t be held in the review. If a discussion is held, the conclusion should always be added to the review as a comment.
  2. It is not the reviewers job to test the code. When reviewing you should be able to assume the code have been tested and is working, and it shouldn’t be needed for the reviewer to know whats going on, as he/she should only look at the code structure etc.
  3. When adding a thread to a review, ask yourself if it actually entitles adding a thread? What entitles making a change when writing a review? It is okay to keep some individuality in the code – keeping comments in the code, or choosing a different solution than the reviewer would - and if the code is working and is secure, it should only be changed if it’s in direct violation of the chosen coding convention.
  4. If the change suggested in the review isn’t because something won’t work, are insecure or similar situations, the reviewer must add “NIT” at the start of the comment. Any threads added this way can be ignored by the assignee, and as few as possible threads marked “NIT” should be added.
  5. A suggestion can be skipped if the assignee have good reasons. But if this is something that should be revisited later on, a Jira task must be created and the Scrum master should be informed so that the task isn’t forgotten.
  6. If an MR has stuff that needs to be changed etc. because of a review. The Jira task should be set to “To-do”, and only when this is handles should this be moved back to “Review”.




Googles definition of a good review

This can be used as inspiration when making a review:

  • Design: Is the code well-designed and appropriate for your system?
  • Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users?
  • Complexity: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future?
  • Tests: Does the code have correct and well-designed automated tests?
  • Naming: Did the developer choose clear names for variables, classes, methods, etc.?
  • Comments: Are the comments clear and useful?
  • Style: Does the code follow our style guides?
  • Documentation: Did the developer also update relevant documentation?

Picking the Best Reviewers

In general, you want to find the best reviewers who are capable of responding to your review within a reasonable period of time.

The best reviewer is the person who will be able to give you the most thorough and correct review for the piece of code you are writing. This usually means the owner(s) of the code, who may or may not be the people in the OWNERS file. Sometimes this means asking different people to review different parts of the CL.

If the ideal reviewer is not available, you should at least CC them on your change.

In-Person Reviews (and Pair Programming)

If you pair-programmed a piece of code with somebody who was qualified to do a good code review on it, then that code is considered reviewed.