Jump to content

MR review and merging guidelines

From postmarketOS Wiki
(Redirected from Rules for merging)

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 requirements

Critical fixes Trivial MRs Any other MRs Move device from category Enable kernel options across multiple kernels
pmaports 1 any approval,
notify maintainer
1 any approval 1 package maintainer approval
+ 1 any approval
1 approval from M + 2 CT + 1 CT/TC/M + minimum 1 week 2 approvals
pmbootstrap 1 any approval,
notify maintainers
(only CT can merge)
1 maintainer approval 1 maintainer approval does not apply does not apply
Other projects
with a maintainer
1 any approval,
notify maintainers
1 any approval 1 maintainer approval does not apply does not apply
Other projects
without a maintainer
1 any approval 1 any approval 1 any approval,
a maintainer shall be
sought
does not apply does not apply

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".

Enable kernel options across multiple kernels: can be done in one merge request, does not need approvals from maintainers because then it would be too much effort to enable kernel options across all kernels. It can be reverted afterwards if there is a good reason.

Examples

Critical fixes:

  • some devices don't boot anymore without a specific fix
  • edge blog posts

Trivial MRs:

  • typo fixes
  • fix broken links
  • aport-kind package upgrades in pmaports

Testing requirements

  • pmaports edge: any person in a MR thread confirms that a MR works
  • pmaports stable: one person from the team confirms that a MR works
    • device specific MRs that the team can't test: require confirmation of device maintainer that it works on stable

Backporting

Backporting features from pmaports edge to stable is done in service packs. So the new feature has some additional testing time in edge, before it potentially breaks something in stable.

Critical fixes can be backported immediately, after proper testing to make sure that there is no breakage.

All patches for stable branches must go through edge first and get backported from there. The only exception are patches that fix build failures that only happen on stable.