GitLab Documentation

Code Review Guidelines

Getting your merge request reviewed, approved, and merged

There are a few rules to get your merge request accepted:

  1. Your merge request should only be merged by a maintainer.
    1. If your merge request includes only backend changes [1], it must be approved by a backend maintainer.
    2. If your merge request includes only frontend changes [1], it must be approved by a frontend maintainer.
    3. If your merge request includes UX changes [1], it must be approved by a UX team member.
    4. If your merge request includes adding a new JavaScript library [1], it must be approved by a frontend lead.
    5. If your merge request includes adding a new UI/UX paradigm [1], it must be approved by a UX lead.
    6. If your merge request includes frontend and backend changes [1], it must be approved by a frontend and a backend maintainer.
    7. If your merge request includes UX and frontend changes [1], it must be approved by a UX team member and a frontend maintainer.
    8. If your merge request includes UX, frontend and backend changes [1], it must be approved by a UX team member, a frontend and a backend maintainer.
    9. If your merge request includes a new dependency or a filesystem change, it must be approved by a Build team member. See how to work with the Build team for more details.
  2. To lower the amount of merge requests maintainers need to review, you can ask or assign any reviewers for a first review.
    1. If you need some guidance (e.g. it's your first merge request), feel free to ask one of the Merge request coaches.
    2. The reviewer will assign the merge request to a maintainer once the reviewer is satisfied with the state of the merge request.

For more guidance, see CONTRIBUTING.md.

Best practices

This guide contains advice and best practices for performing code review, and having your code reviewed.

All merge requests for GitLab CE and EE, whether written by a GitLab team member or a volunteer contributor, must go through a code review process to ensure the code is effective, understandable, and maintainable.

Any developer can, and is encouraged to, perform code review on merge requests of colleagues and contributors. However, the final decision to accept a merge request is up to one the project's maintainers, denoted on the engineering projects.

Everyone

Having your code reviewed

Please keep in mind that code review is a process that can take multiple iterations, and reviewers may spot things later that they may not have seen the first time.

Reviewing code

Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then:

The right balance

One of the most difficult things during code review is finding the right balance in how deep the reviewer can interfere with the code created by a reviewee.

GitLab-specific concerns

GitLab is used in a lot of places. Many users use our Omnibus packages, but some use the Docker images, some are installed from source, and there are other installation methods available. GitLab.com itself is a large Enterprise Edition instance. This has some implications:

  1. Query changes should be tested to ensure that they don't result in worse performance at the scale of GitLab.com:
    1. Generating large quantities of data locally can help.
    2. Asking for query plans from GitLab.com is the most reliable way to validate these.
  2. Database migrations must be:
    1. Reversible.
    2. Performant at the scale of GitLab.com - ask a maintainer to test the migration on the staging environment if you aren't sure.
    3. Categorised correctly:
      • Regular migrations run before the new code is running on the instance.
      • Post-deployment migrations run after the new code is deployed, when the instance is configured to do that.
      • Background migrations run in Sidekiq, and should only be done for migrations that would take an extreme amount of time at GitLab.com scale.
  3. Sidekiq workers cannot change in a backwards-incompatible way:
    1. Sidekiq queues are not drained before a deploy happens, so there will be workers in the queue from the previous version of GitLab.
    2. If you need to change a method signature, try to do so across two releases, and accept both the old and new arguments in the first of those.
    3. Similarly, if you need to remove a worker, stop it from being scheduled in one release, then remove it in the next. This will allow existing jobs to execute.
    4. Don't forget, not every instance will upgrade to every intermediate version (some people may go from X.1.0 to X.10.0, or even try bigger upgrades!), so try to be liberal in accepting the old format if it is cheap to do so.
  4. Cached values may persist across releases. If you are changing the type a cached value returns (say, from a string or nil to an array), change the cache key at the same time.
  5. Settings should be added as a last resort. If you're adding a new setting in gitlab.yml:
    1. Try to avoid that, and add to ApplicationSetting instead.
    2. Ensure that it is also added to Omnibus.
  6. Filesystem access can be slow, so try to avoid shared files when an alternative solution is available.

Credits

Largely based on the thoughtbot code review guide.


Return to Development documentation