@@ -89,7 +89,7 @@ that it does so in the most appropriate way, that it satisfies all requirements,
and that there are no remaining bugs, logical problems, uncovered edge cases,
or known vulnerabilities. The best way to do this, and to avoid unnecessary
back-and-forth with reviewers, is to perform a self-review of your own merge
request, following the [Code Review](#reviewing-code) guidelines.
request, following the [Code Review](#reviewing-a-merge-request) guidelines.
To reach the required level of confidence in their solution, an author is expected
to involve other people in the investigation and implementation processes as
...
...
@@ -129,7 +129,7 @@ This
### The responsibility of the reviewer
[Review the merge request](#reviewing-code) thoroughly. When you are confident
[Review the merge request](#reviewing-a-merge-request) thoroughly. When you are confident
that it meets all requirements, you should:
- Click the Approve button.
...
...
@@ -211,7 +211,7 @@ Instead these should be sent to the [Release Manager](https://about.gitlab.com/c
mentioning them; this will ensure they see it if their notification level is
set to "mentioned" and other people will understand they don't have to respond.
### Having your code reviewed
### Having your merge request reviewed
Please keep in mind that code review is a process that can take multiple
iterations, and reviewers may spot things later that they may not have seen the
...
...
@@ -244,52 +244,17 @@ first time.
If you want to have your merge request reviewed, you can assign it to any reviewer. The list of reviewers can be found on [Engineering projects](https://about.gitlab.com/handbook/engineering/projects/) page.
You can also use `ready for review` label. That means that your merge request is ready to be reviewed and any reviewer can pick it. It is recommended to use that label only if there isn't time pressure and make sure the merge request is assigned to a reviewer.
You can also use `workflow::ready for review` label. That means that your merge request is ready to be reviewed and any reviewer can pick it. It is recommended to use that label only if there isn't time pressure and make sure the merge request is assigned to a reviewer.
When your merge request was reviewed and can be passed to a maintainer you can either pick a specific maintainer or use a label `ready for merge`.
It is responsibility of the author of a merge request that the merge request is reviewed. If it stays in `ready for review` state too long it is recommended to assign it to a specific reviewer.
### List of merge requests ready for review
#### List of merge requests ready for review
Developers who have capacity can regularly check the list of [merge requests to review](https://gitlab.com/groups/gitlab-org/-/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&label_name%5B%5D=ready%20for%20review) and assign any merge request they want to review.
Developers who have capacity can regularly check the list of [merge requests to review](https://gitlab.com/groups/gitlab-org/-/merge_requests?state=opened&label_name%5B%5D=workflow%3A%3Aready%20for%20review) and assign any merge request they want to review.
### Review turnaround time
Since [unblocking others is always a top priority](https://about.gitlab.com/handbook/values/#global-optimization),
reviewers are expected to review assigned merge requests in a timely manner,
even when this may negatively impact their other tasks and priorities.
Doing so allows everyone involved in the merge request to iterate faster as the
context is fresh in memory, and improves contributors' experience significantly.
#### Review-response SLO
To ensure swift feedback to ready-to-review code, we maintain a `Review-response` Service-level Objective (SLO). The SLO is defined as:
> - review-response SLO = (time when first review response is provided) - (time MR is assigned to reviewer) < 2 business days
If you don't think you'll be able to review a merge request within the `Review-response` SLO
time frame, let the author know as soon as possible and try to help them find
another reviewer or maintainer who will be able to, so that they can be unblocked
and get on with their work quickly.
If you think you are at capacity and are unable to accept any more reviews until
some have been completed, communicate this through your GitLab status by setting
the `:red_circle:` emoji and mentioning that you are at capacity in the status
text. This will guide contributors to pick a different reviewer, helping us to
this through your GitLab.com Status, authors are expected to realize this and
find a different reviewer themselves.
When a merge request author has been blocked for longer than
the `Review-response` SLO, they are free to remind the reviewer through Slack or assign
another reviewer.
### Reviewing code
### Reviewing a merge request
Understand why the change is necessary (fixes a bug, improves the user
experience, refactors the existing code). Then:
...
...
@@ -310,26 +275,47 @@ experience, refactors the existing code). Then:
"LGTM :thumbsup:", or "Just a couple things to address."
- Assign the merge request to the author if changes are required following your
review.
- Set the milestone before merging a merge request.
- Ensure the target branch is not too far behind master. If
[master is red](https://about.gitlab.com/handbook/engineering/workflow/#broken-master),
it should be no more than 100 commits behind.
### Merging a merge request
Before taking the decision to merge:
- Set the milestone.
- Consider warnings and errors from danger bot, code quality, and other reports.
Unless a strong case can be made for the violation, these should be resolved
before merge.
- Ensure a passing CI pipeline or if [master is broken](https://about.gitlab.com/handbook/engineering/workflow/#broken-master), post a comment mentioning the failure happens in master with a
link to the ~"master:broken" issue.
- Avoid accepting a merge request before the job succeeds. Of course, "Merge
When Pipeline Succeeds" (MWPS) is fine.
- If you set the MR to "Merge When Pipeline Succeeds", you should take over
subsequent revisions for anything that would be spotted after that.
Unless a strong case can be made for the violation, these should be resolved
before merge. A comment must to be posted if the MR is merged with any failed job.
When ready to merge:
- Consider using the [Squash and
merge][squash-and-merge] feature when the merge request has a lot of commits.
feature when the merge request has a lot of commits.
When merging code a maintainer should only use the squash feature if the
author has already set this option or if the merge request clearly contains a
messy commit history that is intended to be squashed.
-**Start a new merge request pipeline with the `Run Pipeline` button in the merge
request's "Pipelines" tab, and enable "Merge When Pipeline Succeeds" (MWPS).** Note that:
- If the **latest [Pipeline for Merged Results](../ci/merge_request_pipelines/pipelines_for_merged_results/#pipelines-for-merged-results-premium)** finished less than 2 hours ago, you
might merge without starting a new pipeline as the merge request is close
enough to `master`.
- If the merge request is from a fork, check how far behind `master` the
source branch is. If it's more than 100 commits behind, ask the author to
rebase it before merging.
- If [master is broken](https://about.gitlab.com/handbook/engineering/workflow/#broken-master),
in addition to the two above rules, check that any failure also happens
in `master` and post a link to the ~"master:broken" issue before clicking the
red "Merge" button.
- When you set the MR to "Merge When Pipeline Succeeds", you should take over
subsequent revisions for anything that would be spotted after that.
this through your GitLab.com Status, authors are expected to realize this and
find a different reviewer themselves.
When a merge request author has been blocked for longer than
the `Review-response` SLO, they are free to remind the reviewer through Slack or assign
another reviewer.
## Examples
How code reviews are conducted can surprise new contributors. Here are some examples of code reviews that should help to orient you as to what to expect.
Rémy Coutable <remy@rymai.me>