All non-trivial merge requests should be reviewed and merged by someone other than the author. A merge request that touches code is never trivial, but one that fixes a typo in the documentation probably is.
Trivial updates, such as docs updates, do not require a logged issue.
All team members are encouraged to review community contributions. However, each MR should have a single accountable reviewer, who is also approved as a CODEOWNER. That reviewer can ask others in the team for feedback but they are solely accountable for the merge/approval decision.
If you are assigned as primary reviewer and do not feel confident in your ability to approve and merge, it is your responsibility to either (a) request assist from a team member on specific parts of the code, or (b) ask another team member to take the role of primary reviewer.
MR approvals are set (on a per-repo basis) to not use the option to
Remove all approvals when commits are added to source branch. This means approvals are “sticky” and can be requested any time during the review cycle. This also means it is the Merger’s responsibility to check commit history and raise an alarm on any regressions or other concerns introduced after another team member granted their approval.
Security Note: In most cases, the closing “post-approval” tasks should be cosmetic - such as docs, linting, and changelog updates - but team members should nevertheless be on the lookout for any regressions or malicious-looking code that may have been added after approvals are given and before the Merge is applied. (If in doubt, please request a repeat-review from other approvers on the MR.)
Team authored MRs may be reviewed by any other team member, but should also be approved by a Manager (probably AJ), as described below.
For community contributions, the community contributor should indicate readiness to merge and the core team member (primary reviewer) will approve the MR and also perform the merge.
All Community-Contributed MRs should have their corresponding Issue marked with the
Community-Contributed MR label in Gitlab. This helps in prioritization of code contributions. We aim to be responsive in all Community-Contributed MRs, as a sign of respect for the community members’ contributed time and effort.
The first team member to review should assign themselves to the review and check the following are present:
Note: If not comfortable being primary approver, due to either time constraints or subject matter expertise, the first reviewer should request review by tagging another team member.
Awaiting Action::Authorlabel. Sparingly and with due respect, we may ping a contributor on Slack (in DM or in the
#contributingchannel) to notify them of pending action on their side.
For both Community-Contributed MRs and Team-Authored MRs, a Manager-Level approval is required for any non-trivial updates - in addition to Team Member approval. This can be requested either when the MR foundation is in place or as a “final check”. The manager-level approval should generally be requested after the MR is otherwise “clean” - and after known action items and questions are called out in the text of the MR.
The goal in the dual-approval approach is to create a virtuous cycle of individual ownership combined with manager-level accountability, while fostering organic and supportive training opportunities for new team members.
As experts catch issues in MRs that the original reviewers did not, we will update this section and the Contributor Guide, and reviewers will learn new things to look out for until they catch (almost) everything the expert would, at which points they will be experts themselves.