Skip to content

Merge requests

Tip

GitLab has extensive documentation on merge requests which is worth reading for background.

Merge requests (MRs) are the primary mechanism by which code changes are integrated into our services. We use Merge Requests because they give us an opportunity to present and discuss code changes.

Draft Merge Request

Draft merge requests are useful when you want to solicit discussion on a piece of code without proposing it for merging. A MR may be marked as draft by prefixing the summary with "Draft:".

When working on an issue, you may have questions about how to proceed or wish to have some commentary on code before it's finished to check you're heading in the right direction. Draft MRs are useful since they allow discussion on code but telegraph that the code should not be reviewed.

If opening a draft MR, indicated in the description why it has been opened. Examples:

  • "I'm unsure if using Cloud Pub/Sub over Cloud Tasks is appropriate in this case. I've added some comments in main.tf indicating where I've have to work-around Cloud Pub/Sub's permissions model. Does anyone have an opinion?".
  • "This MR indicates the direction I'm heading in with this story but I'm not sure if this actually fixes the problem. Could someone quickly test this branch and see if it fixes the issue in their opinion?"

Roles

MRs will by their very nature involve multiple people:

  • The author of the MR is usually the person who wrote the code and opened the MR. They usually apply the workflow::review required label to the related issue.
  • A reviewer can be any member of the team who is not the author. Usually it is the reviewer which approves the MR and applies workflow::needs testing or workflow::rework labels.
  • A tester can be any member of the team who is not the author. Often the reviewer and tester are the same person but not always. Usually it is the tester which merges the MR. The tester may also apply the workflow::rework label if there is an issue with the code which arises in testing.
  • Any member of the team may assign the workflow::blocked label to the related issue if there is a pending question or external blocker which stops the MR being reviewed or tested. It is good practice to draw attention to the MR when labelling the related issue explaining which MR is stopping progress on the issue and why.

Discussions

When commenting on MRs, bear in mind that the audience for your comment will stretch beyond the author of the MR itself. Comments should be useful for those members of the team coming to the MR afresh or, in the case of public or internal projects, people outside of the team coming in to see the state of the MR.

As such, err on the side of adding too much context to a MR rather than too little. Remember that any context which is not recording in the MR will not be available to those visiting the MR at a later date.

Guidelines

  • Giving good code-review is an art form. The RedHat developers' blog has a good article with some tips for constructive reviews.
  • If referring to in-person discussion, add a brief note of the discussion in the MR. Example: "We had a chat about this earlier today and decided to proceed without addressing this point since we believed it was a separate issue. I've opened the issue at #[issue number]"
  • If there is a pending decision or question in a MR discussion, add a note to the related issue indicating which MR to look at. This is to allow people picking up issues from the board to see which MRs currently have an active discussion.
  • When reviewing a MR, if a new version of the MR diff resolves one of your code-review comments mark the comment as resolved. This helps reduce the visual noise in the MR diff.