diff options
Diffstat (limited to 'doc/development/code_review.md')
| -rw-r--r-- | doc/development/code_review.md | 68 |
1 files changed, 48 insertions, 20 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 29e2aa1a581..b7d74b17eb3 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -45,9 +45,9 @@ page, with these behaviours: 1. It will not pick people whose [GitLab status](../user/profile/index.md#current-status) contains the string 'OOO'. -2. [Trainee maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#trainee-maintainer) +1. [Trainee maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#trainee-maintainer) are three times as likely to be picked as other reviewers. -3. It always picks the same reviewers and maintainers for the same +1. It always picks the same reviewers and maintainers for the same branch name (unless their OOO status changes, as in point 1). It removes leading `ce-` and `ee-`, and trailing `-ce` and `-ee`, so that it can be stable for backport branches. @@ -58,18 +58,21 @@ As described in the section on the responsibility of the maintainer below, you are recommended to get your merge request approved and merged by maintainer(s) from teams other than your own. - 1. If your merge request includes backend changes [^1], it must be - **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**. - 1. If your merge request includes frontend changes [^1], it must be - **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**. - 1. If your merge request includes UX changes [^1], it must be - **approved by a [UX team member][team]**. - 1. If your merge request includes adding a new JavaScript library [^1], it must be - **approved by a [frontend lead][team]**. - 1. If your merge request includes adding a new UI/UX paradigm [^1], it must be - **approved by a [UX lead][team]**. - 1. If your merge request includes a new dependency or a filesystem change, it must be - **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details. +1. If your merge request includes backend changes [^1], it must be + **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**. +1. If your merge request includes database migrations or changes to expensive queries [^2], it must be + **approved by a [database maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_database)**. + Read the [database review guidelines](database_review.md) for more details. +1. If your merge request includes frontend changes [^1], it must be + **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**. +1. If your merge request includes UX changes [^1], it must be + **approved by a [UX team member][team]**. +1. If your merge request includes adding a new JavaScript library [^1], it must be + **approved by a [frontend lead][team]**. +1. If your merge request includes adding a new UI/UX paradigm [^1], it must be + **approved by a [UX lead][team]**. +1. If your merge request includes a new dependency or a filesystem change, it must be + **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details. #### Security requirements @@ -205,9 +208,9 @@ first time. - Extract unrelated changes and refactorings into future merge requests/issues. - Seek to understand the reviewer's perspective. - Try to respond to every comment. -- The merge request author resolves only the discussions they have fully - addressed. If there's an open reply, an open discussion, a suggestion, - a question, or anything else, the discussion should be left to be resolved +- The merge request author resolves only the threads they have fully + addressed. If there's an open reply, an open thread, a suggestion, + a question, or anything else, the thread should be left to be resolved by the reviewer. - Push commits based on earlier rounds of feedback as isolated commits to the branch. Do not squash until the branch is ready to merge. Reviewers should be @@ -317,7 +320,7 @@ reviewee. ### GitLab-specific concerns GitLab is used in a lot of places. Many users use -our [Omnibus packages](https://about.gitlab.com/installation/), but some use +our [Omnibus packages](https://about.gitlab.com/install/), but some use the [Docker images](https://docs.gitlab.com/omnibus/docker/), some are [installed from source](../install/installation.md), and there are other installation methods available. GitLab.com itself is a large @@ -339,8 +342,7 @@ Enterprise Edition instance. This has some implications: - [Background migrations](background_migrations.md) run in Sidekiq, and should only be done for migrations that would take an extreme amount of time at GitLab.com scale. -1. **Sidekiq workers** - [cannot change in a backwards-incompatible way](sidekiq_style_guide.md#removing-or-renaming-queues): +1. **Sidekiq workers** [cannot change in a backwards-incompatible way](sidekiq_style_guide.md#removing-or-renaming-queues): 1. Sidekiq queues are not drained before a deploy happens, so there will be workers in the queue from the previous version of GitLab. 1. If you need to change a method signature, try to do so across two releases, @@ -363,6 +365,31 @@ Enterprise Edition instance. This has some implications: 1. **Filesystem access** can be slow, so try to avoid [shared files](shared_files.md) when an alternative solution is available. +## 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. + +**["Modify `DiffNote` to reuse it for Designs"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13703):** +It contained everything from nitpicks around newlines to reasoning +about what versions for designs are, how we should compare them +if there was no previous version of a certain file (parent vs. +blank `sha` vs empty tree). + +**["Support multi-line suggestions"](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25211)**: +The MR itself consists of a collaboration between FE and BE, +and documenting comments from the author for the reviewer. +There's some nitpicks, some questions for information, and +towards the end, a security vulnerability. + +**["Allow multiple repositories per project"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10251)**: +ZJ referred to the other projects (workhorse) this might impact, +suggested some improvements for consistency. And James' comments +helped us with overall code quality (using delegation, `&.` those +types of things), and making the code more robust. + +**["Support multiple assignees for merge requests"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10161)**: +A good example of collaboration on an MR touching multiple parts of the codebase. Nick pointed out interesting edge cases, James Lopes also joined in raising concerns on import/export feature. + ### Credits Largely based on the [thoughtbot code review guide]. @@ -377,3 +404,4 @@ Largely based on the [thoughtbot code review guide]. [team]: https://about.gitlab.com/team/ [build handbook]: https://about.gitlab.com/handbook/build/handbook/build#how-to-work-with-build [^1]: Please note that specs other than JavaScript specs are considered backend code. +[^2]: We encourage you to seek guidance from a database maintainer if your merge request is potentially introducing expensive queries. It is most efficient to comment on the line of code in question with the SQL queries so they can give their advice. |