diff options
author | Nick Thomas <nick@gitlab.com> | 2019-04-17 10:14:35 +0000 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-04-17 10:14:35 +0000 |
commit | 128f91fce7a62c7f6c444e670a00294ce1b777a1 (patch) | |
tree | 4ea910235ddf34e35322f9dde320d092fd0644a4 | |
parent | 2811ace5738cae31f766aae41020ecd3332617c3 (diff) | |
parent | 95dbc34e29876b9c6f2eaecfd45f9c4ac9824ca7 (diff) | |
download | gitlab-ce-128f91fce7a62c7f6c444e670a00294ce1b777a1.tar.gz |
Merge branch 'dm-code-review-sla' into 'master'
Rewrite "Review turnaround time" section in light of Global Optimization value
See merge request gitlab-org/gitlab-ce!27411
-rw-r--r-- | doc/development/code_review.md | 39 |
1 files changed, 31 insertions, 8 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index b1a32e0ed26..4e2213f7742 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -132,14 +132,6 @@ If a developer who happens to also be a maintainer was involved in a merge reque as a domain expert and/or reviewer, it is recommended that they are not also picked as the maintainer to ultimately approve and merge it. -Try to review in a timely manner; doing so allows everyone involved in the merge -request to iterate faster as the context is fresh in memory. Further, this -improves contributors' experiences significantly. Reviewers should aim to review -within two working days from the date they were assigned the merge request. If -you don't think you'll be able to review a merge request within that time, let -the author know as soon as possible. When the author of the merge request has not -heard anything after two days, a new reviewer should be assigned. - Maintainers should check before merging if the merge request is approved by the required approvers. @@ -220,6 +212,37 @@ It is responsibility of the author of a merge request that the merge request is 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. +### 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, improves contributors' experiences significantly, +and gives authors more time to address feedback and iterate on their work before +the [feature freeze](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md#feature-freeze-on-the-7th-for-the-release-on-the-22nd). + +A turnaround time of two working days is usually acceptable, since engineers +will typically have other things to work on while they're waiting for review, +but don't hesitate to ask the author if it's unclear what time frame would be +acceptable, how urgent the review is, or how significant the blockage. Authors +are also encouraged to provide this information up-front to reviewers, but are +expected to be mindful of the [guidelines on when to ask for review on MRs that +are intended to go in before the feature freeze](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md#between-the-1st-and-the-7th), +and realistic in their expectations if these were not followed. + +If you don't think you'll be able to review a merge request within a reasonable +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. Of course, if you are out of office and have +[communicated](https://about.gitlab.com/handbook/paid-time-off/#communicating-your-time-off) +this through your GitLab.com Status, authors are expected to realize this and +find a different reviewer themselves. + +When a merge request author feels like they have been blocked for longer than +is reasonable, they are free to remind the reviewer through Slack or assign +another reviewer. + ### Reviewing code Understand why the change is necessary (fixes a bug, improves the user |