Code Review Guidelines

Code reviews are mandatory for every merge request, you should get familiar with and follow our Code Review Guidelines.

These guidelines also describe who would need to review, approve and merge your merge request.

Values

Every reviewer at Bramble must strive for our reviewer values.

Reviewer

All Bramble engineers can, and are encouraged to, perform a code review on merge requests of colleagues and community contributors. If you want to review merge requests, you can wait until someone assigns you one, but you are also more than welcome to browse the list of open merge requests and leave any feedback or questions you may have.

You can find someone to review your merge requests by looking on the team page, or on the list of Bramble Engineering Projects.

You can also help community contributors get their merge requests ready, by becoming a Merge Request Coach.

Note that while all engineers can review all merge requests, the ability to accept merge requests is restricted to maintainers.

Maintainer

Maintainers are Bramble engineers who are experts at code review, know the Bramble product and codebase very well, and are empowered to accept merge requests in one or several Bramble Engineering Projects.

Every project has at least one maintainer, but most have multiple. Some projects have separate maintainers for different specialties. For example, Bramble has separate maintainers for frontend, backend, and database.

Great engineers are often also great reviewers, but code review is a skill in and of itself and not every engineer, no matter their seniority, will have had the same opportunities to hone that skill. It’s also important to note that a big part of being a good maintainer comes from knowing the existing product and codebase extremely well, which lets them spot inconsistencies, edge cases, or non-obvious interactions with other features that would otherwise be missed easily.

To protect and ensure the quality of the codebase and the product as a whole, people become maintainers only once they have convincingly demonstrated that their reviewing skills are at a comparable level to those of existing maintainers.

As with regular reviewers, maintainers can be found on the team page, or on the list of Bramble Engineering Projects.

Meeting the reviewer/maintainer

Communication happens easier when you are familiar with the person reviewing the code. Take opportunities (for example coffee chats) to get to know reviewers to break the ice and facilitate future communication.

How to become a project maintainer

This applies specifically to backend, frontend and database maintainers. Other areas (docs, etc.) may have separate processes.

As a reviewer, a great way to improve your reviewing skills is to participate in MRs. Add your review notes, pass them on to maintainers, and follow the conversation until the MR is closed. If a comment doesn’t make sense to you, ask the commenter to explain further. If you missed something in your review, figure out why you didn’t see it, and note it down for next time.

We have two guidelines for project maintainership, but no concrete rules:

In general, the further along in their career someone is, the more we expect them to be capable of becoming a maintainer.
Maintainers should have an advanced understanding of the Bramble project that they wish to maintain. Before applying for maintainership, a person should get a good feel for the project codebase, expertise in one or more domains, and deep understanding of our coding standards.

Apart from that, someone can be considered as a project maintainer when both:

The MRs they’ve reviewed consistently make it through maintainer review without significant additionally required changes.
The MRs they’ve written consistently make it through reviewer and maintainer review without significant required changes.

Once those are done, they should:

Create a MR to add the maintainership for the project they wish to maintain to their team page entry.
Explain in the MR body why they are ready to take on that responsibility.
Use specific examples of recent “maintainer-level” reviews that they have performed. These include but are not limited to:
1. MRs that introduce non-trivial changes (architectural changes, refactors, developer experience upgrades)
2. MRs that improve our quality of the project (tests, performance)
3. MRs that deliver full features (ideally this is done iteratively, so a good maintainer can guide someone to break down large MRs to smaller ones)
4. Note: MRs that introduce very simple changes are good, but should not be the only source of reviews
Assign the MR to their manager and mention the existing maintainers of the relevant project (Bramble App, Bramble Infra, etc) and area (backend, frontend, etc.).

The approval process to follow is:

The MR will remain open for 5 working days. If more than half of the existing maintainers of the relevant engineering group (e.g. backend) approve the MR, and there are no blocking concerns raised during this period, we’ve got ourselves a new maintainer!
If the MR does not have enough approvals after this period, and no blocking concerns, then the manager should mention the existing maintainers again in a new comment as well as in the relevant Slack channel (e.g. #backend) and keep the MR open for 5 more working days.
After the extra period, any missing feedback from a maintainer should be counted as a neutral response and the manager can merge the MR given no blocking concerns were raised!
Normal non-blocking feedback should be submitted as comments to the MR. To support Bramble Values and avoid groupthink, any blocking negative feedback should be instead submitted to the manager of the aspiring maintainer. The manager will then share the feedback with the aspiring maintainer and post a generic message with a “TL;DR” in the MR.
If there are blocking concerns raised at any point in the approval process, the maintainers who raised the concern should actively work with the maintainer nominee’s manager to develop a plan on how to resolve the concern.

Since the manager of the new maintainer is the MR assignee, they should be the one merging the MR.

It is helpful if the person merging the MR also leaves a comment with a summary.

When the manager merges the MR, they should announce this change in the applicable channels listed under keeping yourself informed section of the engineering handbook and #team-engineering.

The existing maintainers of the relevant engineering group and project will also raise any areas for growth on the merge request. If there are many gaps, the reviewer will need to address these before asking for reconsideration.

Maintainer ratios

We aim to keep the engineer : maintainer ratio under 6, for both frontend and backend. We track this in the [Engineer : Maintainer Ratio dashboard][dashboard]:

Maintainer load

We aim to keep the merge request per maintainer type at a reasonable level as well.

Domain Experts

Our Code Review Guidelines state that we default to assigning reviews to team members with domain expertise.

What makes a domain expert?

We currently don’t provide rigid rules for what qualifies a team member as a domain expert and instead we use a boring solution of implicit and self-identification.

Implicit:

Team members working in a specific stage/group (e.g. Create: Source Code) are implicitly considered domain experts for that area of the app they work on.
Team members working on a specific feature (e.g. search) are implicitly considered domain experts for that feature.

Self-identification:

Team members can self-identify as a domain expert for a specific feature (e.g. file uploads).
Team members can self-identify as a domain expert for a specific technology (e.g. Erlang), product feature (e.g. file uploads) or area of the codebase (e.g. CI).

How to self-identify as a domain expert

The only requirement to be considered a domain expert is to have substantial experience with a specific technology, product feature or area of the codebase. We leave it up to the team member to decide whether they meet this criteria.

Define a new, or use an existing domain expertise key, located in data/domain_expertise.yml.
Update your entry in your own team page entry with a new domain_expertise property and list all domain expertise keys.

When self-identifying as a domain expert, it is recommended to assign the MR to be merged by an already established Domain Expert or a corresponding Engineering Manager.

Where can I find a list of people with domain expertise?

The expertise of a team member can be seen on the Engineering Projects page.

First-response SLO

To ensure swift feedback to ready-to-review code, we maintain a first-response Service-level Objective (SLO). The SLO is defined as:

first-response SLO = (time when first response is provided) - (time MR is submitted and no longer marked WIP) < 2 business days

Managing expectation

When you are assigned to review an MR and you are not able to get to it within the First-response SLO, you should leave a comment on the MR informing the author of your delayed response. If possible, you should also indicate when the author can expect your feedback or help them find an alternative reviewer.

As the author of an MR you should reassign to another reviewer or maintainer if the First-response SLO has not been met and you have been unable to contact the assignee.