Code Review / Merge

Code Review Guidelines

We encourage everybody in the community, with or without merge rights, but with expertise or interest in a topic, to help review code changes. To make sure we keep a healthy community and review culture, the following guidelines should be followed:

  • Be compassionate, especially when interacting with someone new. It might be their first ever MR and a lot of comments could be overwhelming. Allowing small things to slip by in favour of getting someones’ MR merged might motivate them to continue contributing – it doesn’t have to be perfect the first time.

  • Encourage the author. The “review” button on gitlab allows you to leave a comment at the end of your review, this is a great opportunity to write a quick “Thanks for making this MR!”.

  • Try not to argue over details with other reviewers. If there is a disagreement it may be better to take it over to the matrix channel and discuss it there. The worst case is when there is no clear decision and the author is left wondering what to do.

  • If the MR was already reviewed by 1 more people than minimum required approvers, try ping previous reviewers to follow-up. If possible and there is nothing critical in the MR, avoid re-reviewing it for the minimum+2 time, which can cause great confusion.

Merging Workflow

  • Use mrhlpr, so we have the MR link in the commit body. If “merge-bot” user exists in the project, you can assign MRs to it for merging.

  • Generally, CI is required to pass for merging to be possible. If there are valid reasons to merge with broken CI, it is recommended to instead directly push to the main branch.

  • Review other people’s MRs, and merge their stuff if they have enough approvals (see table below)

Approval Rules

For pmaports, see the pmaports approval rules.

For all other projects, the following applies:

Project

Critical fixes

Trivial MRs

Any other MRs

pmbootstrap

1 any approval, notify maintainers (only CC can merge)

1 maintainer approval

1 maintainer approval

Projects with a maintainer

1 any approval, notify maintainers

1 any approval

1 maintainer approval

Projects without a maintainer

1 any approval

1 any approval

1 any approval, a maintainer shall be sought

Any approval references anybody with permission to approve.

Maintainers are listed in projects.

Approvals from merge request authors do not count. If the maintainer creates a merge request, “1 approval from M” becomes “1 approval from other maintainer, if such maintainer exists, or otherwise 1 any approval”.

Examples

Critical fixes:

  • Some devices don’t boot anymore without a specific fix

  • Edge blog posts

Trivial MRs:

  • Typo fixes

  • Fix broken links

  • Aport-style package upgrades in pmaports