3

I have GitLab for code repo and version tracking, and would like to add a code review process that allows reviewers to review code before they are committed. This includes new projects, as well as changes to existing projects. My development team is small (<20 people).

I'm unfamiliar with code review tools, so I would like to know what my options are, and what are their pros and cons? Some quick research brings up the following options:

  1. Atlassian Crucible
  2. Review Board
  3. SmartBear Collaborator
  4. Gitlab Merge requests
4

I've used and assisted in deployment of SmartBear Collaborator, the built-in GitLab Merge requests, and Review Board both with and without integration into GitLab. I've also personally investigated Atlassian Crucible.
The solution you choose is largely based on your workflow, each of the options has it's own benefits and detriments.
There are a few considerations that are common, regardless of which tool you pick.

If the tool is outside of GitLab, you have to worry about how it's linked to the code base. Collaborator and Review Board both operate by running the diff commands on a client side and uploading before and after versions of the files for each change set you push for review. In git-terms, that means it does a git diff --names-only on the hash range you're specifying for the review (sometimes it auto-detects this range, and for GitLab integration the range is <target-branch>...<merge-request-branch>), then uploads all the resulting files from the before and after sides of what would be the resulting diff. This means that if something goes wrong with your integration between the servers hosting GitLab and the your review tool, the review tool may never get a diff and won't ever know it should have.

Whether you're self-hosting GitLab or using the cloud-hosted GitLab makes a difference, especially if you're planning on using one of the code review tools that isn't the built-in GitLab one. You need to be aware of the network access requirements, e.g. cloud-hosted GitLab needs bi-directional network access directly to the server hosting the review tool for any integration to work. Also there have been some changes to the remote integration API for GitLab, i.e. last I checked Review Board hadn't been updated to support the "new" (3 years old) authentication API for GitLab, and if that changes it may break integration until your review tool gets updated. In self-hosted GitLab you can control these changes by limiting when/if you update GitLab, but in the cloud version of GitLab you can't. That said, the integration API has been pretty stable for a while now so if it works now it will probably work for the forseeable future.

Rebasing and merging during review process is a consistent problem for all review tools. Many people gravitate to the post-commit pre-push review style, which is what the Merge Request logic is based on. Doing one of these two is usually required if you have Continuous Integration tools running on Merge Requests prior to merging them. The rebasing method is especially difficult on review tools because it re-creates all commits in your feature branch, giving them an updated date and having no link back to the original commits. Merging causes problems for review tools that aren't directly linked to the code base, because they all take before and after snapshots of the modified files and usually rely on upload order to indicate the order in which changes were made.

Work Flow

The biggest considerations in your workflow are:

  1. Pre-commit or post-commit reviews?
  2. Pre-push (while merge request is open) or post-push (after merge request is closed) reviews?
  3. Reviewers examining in parallel or series? 4, How are fixes for problems found during review resolved?
  4. Is documentation or non-code related changes included in the review? If yes, is that kept with the code repo, or separate?
  5. Do you have needs/requirements on how many people must review the code?
  6. Is a review required, optional, or for later reference only?
  7. Do you want/need feedback linked-to or separated-by the individuals that participate in the review?
  8. What statuses do you need from reviewers (e.g. reviewed some and didn't finish, reviewed some and finished, reviewed all and finished, reviewed some and approve of all changes, reviewed all and approve of all changes, reviewed some and approval is waiting on some bugs, reviewed all and approval is waiting on bugs)?
  9. What notifications do you want from reviews (e.g. added to review, required for review, optional on review, review present, reminder of review, reminder of required for review, review blocked by one or more reviewers not responding)?
  10. What roles do you have in a review (e.g. notification only, must participate in review, optional participant in review)?
  11. How much history/tracking do you want/need from the review (e.g. history of all interaction may be audited for Aerospace agency certification)?
  12. If you're doing post-commit, pre-push reviews, do you allow/require rebase during review, merge during review, or neither (usually one of the two is required if you have Continuous Integration tools)?
  13. Does your repo contain committed symlinks?
  14. Do you need to see/modify/review *nix file permissions?
  15. How often do you move files within your repos?

Tools

Review Board

This is a free and open source review tool created and used by Amazon. It's designed for post-push review, but can technically be used for reviews during any stage. It has some basic plugins available to add a bit of functionality, but is largely as-is. It doesn't really provide any user credential integration as it largely relies on email addresses and doesn't track users directly very much.
It's easy to bring up on a server and start using, the user interface is clean and relatively simple, it allows multiple uploads of changes to the same review, and has an easy way to switch between different uploaded versions of the files so you can compare upload 2 and 6 of one file, then 1 and 7 of another. Since it's designed as a post-push review tool, the approval features are pretty minimal, as is the GitLab integration. GitLab integration hasn't been maintained, and when I tried using it last year I had to hand re-write a block of the GitLab integration code in the Review Board tool for it to work with the then 2 year old GitLab authentication API (so it works out-of-the-box if your repo is fully public, but not so much otherwise). Far and away the largest complaints everyone has with the tool is with the approval status. Anyone, even those not invited to the review, can mark the review as approved, and it cannot be revoked once given. This includes the case where new uploads are added to the review. The approval also isn't per-reviewer, but is a single approval status for the whole review. Approval also isn't limited by whether there are open bugs on the review unless you install an additional plugin. The logging of changes is relatively good on reviews, all actions are added to a running overall discussion list, but you can delete reviews completely (if that's a concern). The nature of the tool as a post-push means it doesn't really try to integrate with GitLab to gate a merge request, it's designed to operate more as documentation of what changes were in the merge request. If you wanted to write your own webhooks and scripts you could probably get the GitLab merge requests to block the merge until the Review Board review is approved, but it would be your own custom scripts to do it.
Overall this is a great tool for post-push review, but I don't think it's very usable as a pre-push merge request code review tool.

SmartBear Collaborator

This is what my company actually uses, and we used it before we used GitLab.
This is a closed source paid commercial tool, though they have a free tier of the tool that has limited functionality and a maximum of 10 users. It's very easy to install an initial evaluation version of the full version of the tool, but gets more complicated if you want things like HTTPS for the Web UI accounts from LDAP or ActiveSync. It does a good job with floating licenses if/when you buy licenses for the full version, so in our case we're sharing 19 licenses among almost 40 users without much of any problems, but the licenses are only for 1 year and aren't very cheap. I've only used the paid version, they changed their paid-vs-free model from a 30-day 10-user free trial model to a separate limited-functionality free install model since I started using it. From what I understand, the free version is identical to the paid version except for the user limit and some of the features not being available in the free version.
Collaborator has lots of configuration options, and it can be a bit overwhelming for the Administrator when you first set it up. It requires you to register users (whether that's through LDAP/Active Directory integration, or manual entry) and has a bit of a Role Based Access model. It's designed to be used as a stand-alone code review tool that doesn't necessarily integrate with anything and works with a bunch of different version control systems. As a result it has a lot of configuration options to enable features that are more useful if you don't have a GitLab merge request and all it's information backing the changes (configurable forms, fields, and selection boxes from an Admin GUI panel for each review). It also allows multiple simultaneous workflow requirements, so it can theoretically be configured to be used for pre-commit, post-commit, pre-push, and post-push workflows. Doing so can be complicated and isn't very well described in documentation if you haven't been using the tool extensively already though (read about templates). This lets you set up to 4 roles in a review (including the author), and you can configure the number of mandatory participants for a role, how many of each role have to Complete/Approve the review (includes all and none options), etc (read the public documentation for configuring the tool). Like Review Board, it also allows you to look at the differences between any two versions of any file that's been uploaded so you can compare version 1 to version 7 of one file, then version 3 and version 4 of another. Unlike Review Board however, this is a little kludgy when using git because the file versions don't seem to stay order consistently with when they were uploaded vs when they were created, and there isn't really any way to link a file version back to the commit hash it appeared in.
Document review is very good in Collaborator if you use standard Microsoft Office/Open Office/Libre Office documents, PDFs, or LaTeX, and you can also review image differences. Most review tools don't really do diffs on these types of things at all, but Collaborator does a decent job and has a very good method of allowing commenting on them (you drop a positional pin rather than commenting on a whole line).
Some of the best features we've found with Collaborator are: You can "checkpoint" the version of a file you last reviewed, and it's a user specific checkpoint that can be used as a reference point for additional future changes. Files are listed in an overall tree view from the main page, with links to file-specific review pages, supporting a much larger set of file changes without being impossible to track with one long scrolling page. Comments can be separated from Defects/Bugs so Defects/Bugs can be required to be addressed and resolved (even if just to say they'll be fixed in a future merge request) before the review can be approved, while comments or discussion points won't necessarily hold it up. When new changes are added it clears all approvals since they haven't reviewed the updated code changes yet.
The biggest problems with Collaborator are: Some of the base assumptions about workflow cannot be changed (it must go Setup->Inspection->Completed, or if everyone finishes reviewing and there are still open defects it cycles from Inspection into Rework, then manually back into Inspection). Code syntax highlighting is still pretty weak and limited to major programming languages (no Make or CMake syntax supported for example). File moves and renames appear as a delete of the old file name and add of the new file name. File permissions are ignored and aren't displayed or examined.
The GitLab integration specifically is working, and works reasonably well. You have to add a GitLab webhook to each repository you want to use with Collaborator, and in Collaborator you have to register the repository. Once you register the repo in Collaborator, you then have to have every user enter their GitLab username for that repo under their individual user settings so Collaborator can match the usernames (GitLab uses human-readable names as usernames rather than login names like all intelligent projects). Once completed, opening a merge request will trigger Collaborator to setup a new review (but not begin it) with the creator of the merge request as the author, all the file changes included in the review, and an entry in their Remote Content section of the review with a link back to the GitLab merge request. If you end up starting the Collaborator review, it will open a pending pipeline for the GitLab merge request (if you have Enterprise edition that supports pipelines), which normally blocks approval of the merge request. Any comments made in the GitLab discussion on the merge request, or on files in the GitLab differences view by users with their GitLab username entered into Collaborator will automatically have the same comments made in the overall discussion or in the overall of the file in Collaborator. Additional pushes to the branch in the merge request automatically trigger a new file upload into Collaborator. When the Collaborator review is terminated, the pipeline on the merge request is marked as failed. If the Collaborator review completes successfully, the pipeline in GitLab is marked as completed successfully.
You'll notice some of the caveats about usernames being configured properly, which gets to be a pain if you use a bunch of different repositories and you're requiring each user in Collaborator to fill out their same Active Directory synchronized human readable name into 40 different repository configurations in their Collaborator user settings. Once it works though, it just works quite well. You can even setup Collaborator to automatically merge merge requests when the reviews complete, and/or close them if the Collaborator reviews are terminated. If you need to re-open a review for some reason though, or if it closed early (sometimes in-hand testing is needed first and you may want to wait the code review on those results), you can't get Collaborator to reopen the pipeline. In fact to all GitLab indications, the Collaborator review is closed once it's closed and cannot be reopened (even though an Admin and under some settings individual users can do that from within Collaborator).
For the purposes of auditing, Collaborator doesn't allow permanent deletion of anything from the GUI by default, which is very important for some people and not for others. You can configure to allow deletion of somethings though. All actions that occur on any/all reviews can be data mined out of it rather easily, and can be exported from the GUI if you care to do so. States of defects are tracked with a running log in the specific defect item about who created it, when it was closed and/or reopened, when new file revisions were uploaded, and any comments/discussion about the defect. You can control who's allowed to close the defect (maybe only the original author or an Admin can?) too. Each user also sees their own version of the state and history of reviews (somewhat), with things new to that user being highlighted for them when they're logged in, and requiring manually marking them as read (or "Mark All as Read").

Built-in GitLab

This is what one of our peer divisions was using when we started implementing GitLab, so I also have used this extensively in practise.
The best thing that can be said of this is that because it's built in it's guaranteed to be up-to-date with the code that's on the branch. As mentioned, all solutions that aren't hard-linked to the repo have the potential to get out-of-sync relative to the actual code in the branch. In my opinion though, this option is only workable if you have very small teams that are communicating extensively on the specifics of changes outside the GitLab tool, don't need quick reviews, and/or limit your set of proposed changes to a single commit (squash each additional set of changes before upload to GitLab).
GitLab runs a long history of everything going on, discussions, uploads, external integration events, etc in the Discussion page of each merge request. This gets very very long quite quickly, and is very hard to go back thru. It's handy for seeing what discussion items are still open that need resolving since they're automatically collapsed in the Discussion view once the individual discussions are closed, but since they're automatically collapsed if the discussion item is closed, it's hard to determine what changes match with the closing of the discussion item (only the open discussion items remain visible in the diff view). Also you have to kludge a way to set a bookmark for yourself on the last version of the files you reviewed by doing something like putting a special comment in the overall discussion that you can search for later. GitLab won't let you even comment on lines that are more than a few lines from one that was changed, so cases where you want to comment about adding an argument to a function because some lines were added to the middle of a function have to be made on or near one of the changed lines instead of on the line that would be affected by the change. That wouldn't be as big a deal if GitLab allowed multiple independent discussion items on the same line of a file. You can only have one, and any other comments on that same line are combined into the one discussion item. Many times a few lines of code change may have many different things wrong with them and may need more discussion items than there are lines of changed code. You end up having to put discussion items on unrelated lines just because they're visible and haven't been used for a discussion item yet since you can't put them on the line that causes the separate discussion (one-per-line limit) or on the lines they affect (outside the default visible area so cannot have discussion items). Luckily, you also can't really have parallel reviews, so hopefully that prevents you from encountering this problem in the first place. Unlike Issues, Merge Requests can only have one assignee. If you make use of the feature to automatically add approvers to all merge requests in a repo, you can guarantee those approvers will ignore most of the emails from GitLab until they're the assignee (notifications options per-user are basically all or nothing, and approvers are automatically subscribed to the merge request they're an approver on from the time they're added). Since the only filterable notification on a merge request that indicates a user needs to take action is for them to be made the assignee, and you can only have one assignee per merge request, you're basically limited to uses where one person does the review and hands it to the next, or you talk outside GitLab and explicitly request when you want things reviewed. This is all if you remember to checkup on your merge requests. Once you're no longer the assignee, it's hard to find the merge requests you were involved in again so you can prod people to actually take care of the ones assigned to them.
For big diffs, GitLab has a lot of problems as well. If you made a small one-line change to a lot of files (replace a common header name?) the webpage GitLab generates to display all the changes crashes some browsers, and nearly locks up others (depending on the number of files). This is because GitLab will collapse single files that have a ton of changes in them and require you to manually expand them, but doesn't have a good way to limit the number of files you see the diffs for at a single time. This creates one hugely long scrolling page that browsers don't handle well.
The GitLab Built-in code review tool is fine if you have a few dedicated reviewers with only a few small-ish reviews over any length of time, and you don't need to speed up how quickly reviews occur (all of this describes a public open source code base with most users only having read access). If any of these isn't true though, the Built-In tool is not for you.

Atlassian Crucible

I haven't tried integrating this with GitLab, but I've done extensive research on exactly this tool. When you read the documentation about this, be very careful to see what limits they set (a limited number of files can be included in a review at a time), and what's actually included in the Crucible tool without the rest of the Atlassian tools (e.g. FishEye, etc). The tool is largely designed to be used with Atlassian's BitBucket and virtually requires you to also have FishEye so you can see more than just the limited context around the diffs. While it's possible to not use Jira for the tracking of defects/discussion items/bugs found during the review, it's not as well supported.

0

Please take a look at RhodeCode

It's pull-request based code-review has some very unique market features such as:

  • versioned pull-requests allowing reviewing changes by diffing only updated to the previous version
  • simple live inline/pull request chat that allows quick discussions on source code
  • default reviewers with file-based rules (match file to a review group), branch rules.
  • allow to pick reviewers based on diff blame. ie. pick author of files you changed
  • mandatory/optional reviewers
  • user group voting rules

It supports Git, Mercurial and Subversion.

The only thing missing is attachments in the comments. It has a Gists/Snippet system so it can be used to create linked data.

  • The original question asked for a review tool to be used with GitLab. RhodeCode doesn't seem to really integrate as a review tool, but is a GitLab replacement. – mtalexan Mar 11 at 22:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.