Merge requests workflow¶

These are suggested guidelines for every project, try to follow them, but apply only the ones you are comfortable with.

Introduction¶

Software development is an art that is very much an individual contribution on a collaborative canvas. Collaboration and knowledge transfer are extremely important so that the team can collectively work as one mind. Code reviews are one of the primary practices to engage the whole team. Code reviews are a key industry practice that helps ensure that the entire team grows along with the code. Some teams work horizontally across database, server, and UI code while others work through different feature areas of the product. Code reviews can help both styles of teams stimulate conversations and learning across the code base.

Why are code reviews valuable?¶

If a single engineer owns an area of code, then that responsibility follows him outside of the office. Having a single team member as the critical path makes the team fragile. Code reviews distribute knowledge across the team.
When bringing someone new into the team, it’s important to do everything possible to help them get up to speed quickly. Code reviews stimulate conversations around code structure, style, and architecture as a natural part of the workday. New team members can more efficiently and organically merge into the team culture, minimizing the cost of onboarding.
Code reviews are an excellent way for the engineer and the reviewer to discover bugs. Walking through a tricky area of code demands that both parties understand the flow of logic and validate the solution.
No team stays static. As the team invests more in the product, better engineering paradigms develop. Code reviews help keep everyone engaged and distributes best practices learned across the team. Engineers connect on a more meaningful technical level, and a better product results. While the best bugs are the ones not coded, code reviews often reveal flaws in logic as a new set of eyes will find things the original engineer didn’t account for.

Scope of a MR¶

When you choose how much work has to be done in a single merge requests, it is important remembering the UNIX’s KISS philosophy: Keep It Simple and Stupid.

To make a review effective, and to make the merge request faster, keep the merge request as little as possible: every merge request should implement just a feature, or fix just an issue.

This has many positive side effects:

Reviewers will be more effective, because they can focus on a single change, without having to spend too much time on it
The process will be faster: less LOC, less errors
There will be less merge conflicts
Bug fixes and features don’t block each other: if you are trying to fix two issues in the same MR, if one is perfect and the other still needs work, you cannot just merge the first bugfix

Gitlab’s settings¶

There are different Gitlab’s settings that can help the team following these guidelines. Here we suggest some of them, but it’s up to the single project’s maintainer choosing which one (s)he want to enforce. We highlight here only the ones that are different from the default settings

Settings -> General -> Merge request settings¶

Merge method: Fast-forward merge. Be aware your developers need to feel comfortable rebasing the code
Merge request approvals -> Approvals required: 1
Only allow merge requests to be merged if the pipeline succeeds: true
Only allow merge requests to be merged if all discussions are resolved: true

Settings -> Repository -> Push Rules¶

Branch name: ^master|(feature|bugfix|hotfix|release)\/(\w|\-)+$

For example, a branch named feature/cool-method is OK, whereas branches like cool_method or feature/cool/method will be automatically rejected when pushed.

Settings -> Repository -> Protected Branches¶

You need to protect master with the following settings:

allowed to merge: Mantainers
allowed to push: No one

For more information, read the official documentation

Opening a new merge request¶

Title and description¶

When you open a MR, assign it a meaningful title and a description.

If the MR is related to an issue, insert the issue number in the title, e.g. #42: Implementing the answer to life, universe and everything. If you want a general review, but the code isn’t ready yet, prepend [WIP] to the tile.

In the description of the MR, remember to add:

An indication of the related issue number, if any. E.g: Resolve(s)/Implement(s)/Fix(es) #42
If the MR is based on another MR, indicate so. E.g: Requires !7
Any information useful for the review

Assignee and approvals¶

If your MR requires review by more than one person (e.g., a data scientist and a developer) add them as “Approvers”. Otherwise, if only one reviewer is necessary, simply assign him/her, unless the project requires you to indicate at least one approvers.

TBD: how to choose a reviewer

Other settings¶

Mark “Remove branch when merged” - keep the repo clean :-)
If you feel comfortable with the option, mark “Squash commits”

Review¶

When a MR is assigned you, you have to review it. Take a look to the code, and start discussions when you think something can be improved. If there is something to discuss that isn’t related to a specific piece of code, start a discussion in the main thread: write a comment, and instead of clicking on “Comment”, click on the down arrow on the right of the comment button, and select “Start discussion”.

When you have finished your review:

if you have opened or continued any discussion, reassign the MR to its author
if everything is good, assigne to next reviewer, if (s)he is specified, or to a maintainer of the project saying it is good to merge.
if you are both a reviewer and a maintainer, merge directly the MR.

If you opened any discussion, the author will improve the code (or explain his/her decisions) and then will reassign the MR to you. You have to review code again and mark discussions as solved.

Improving¶

You, as author, will have to improve your code if reviewers find any flaw. After you fixed something do not resolve related discussions, it’s up to the reviewer choosing if something is good enough after improving.

After you finished improving, assign back to the last reviewer - but remember that if you inserted any approvers, everyone of them have to approve again after there is a code change.

Do not suppose that every discussion is something you have to change - reviewers can get it wrong too! So feel free to discuss and defend your solution.

If you think that any discussion is out of scope for the MR, open a related issue and comment in the discussion with the number and why you think it is out of scope. If the reviewer agrees with you, (s)he will resolve the discussion.

Merging¶

After every reviewer has finished reviewing the code, and there isn’t any discussion left open, the MR is assigned back to the author that can merge in develop (master if the first version hasn’t been reached yet).

If the author hasn’t permissions to merge in master, (s)he will assign the MR to a project’s mantainer, which will merge the MR - the project’s mantainer can do another review, if (s)he wasn’t involved during the standard review, because should be the person who better knows the project. Anyway, it is at his/her own discretion, since someone else has already reviewd it.

Other¶

Work in Progress status¶

Feedback often, feedback early!

Open a MR marked as WIP when you are about around the 30% of work - the reviewer will take a look only to major design, and not to tiny details, there is time to fix them.