summaryrefslogtreecommitdiff
path: root/doc/development/code_review.md
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2020-07-20 12:26:25 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2020-07-20 12:26:25 +0000
commita09983ae35713f5a2bbb100981116d31ce99826e (patch)
tree2ee2af7bd104d57086db360a7e6d8c9d5d43667a /doc/development/code_review.md
parent18c5ab32b738c0b6ecb4d0df3994000482f34bd8 (diff)
downloadgitlab-ce-a09983ae35713f5a2bbb100981116d31ce99826e.tar.gz
Add latest changes from gitlab-org/gitlab@13-2-stable-ee
Diffstat (limited to 'doc/development/code_review.md')
-rw-r--r--doc/development/code_review.md18
1 files changed, 10 insertions, 8 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 9c6cb1d0237..fd53ce79534 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -95,17 +95,16 @@ with [domain expertise](#domain-experts).
**approved by a [Distribution team member](https://about.gitlab.com/company/team/)**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/development/enablement/distribution/#how-to-work-with-distribution) for more details.
1. If your merge request includes documentation changes, it must be **approved
by a [Technical writer](https://about.gitlab.com/handbook/engineering/ux/technical-writing/#designated-technical-writers)**, based on
- the appropriate [product category](https://about.gitlab.com/handbook/product/categories/).
-1. If your merge request includes Quality and non-Quality-related changes (*3*), it must be **approved
+ the appropriate [product category](https://about.gitlab.com/handbook/product/product-categories/).
+1. If your merge request includes end-to-end **and** non-end-to-end changes (*3*), it must be **approved
by a [Software Engineer in Test](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors)**.
-1. If your merge request includes _only_ Quality-related changes (*3*), it must be **approved
- by a [Quality maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_qa)**.
+1. If your merge request only includes end-to-end changes (*3*) **or** if the MR author is a [Software Engineer in Test](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors), it must be **approved by a [Quality maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_qa)**
- (*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.
-- (*3*): Quality-related changes include all files within the `qa` directory.
+- (*3*): End-to-end changes include all files within the `qa` directory.
#### Security requirements
@@ -386,9 +385,12 @@ author.
- Learning how to find the right balance takes time; that is why we have
reviewers that become maintainers after some time spent on reviewing merge
requests.
-- Finding bugs and improving code style is important, but thinking about good
- design is important as well. Building abstractions and good design is what
- makes it possible to hide complexity and makes future changes easier.
+- Finding bugs is important, but thinking about good design is important as
+ well. Building abstractions and good design is what makes it possible to hide
+ complexity and makes future changes easier.
+- Enforcing and improving [code style](contributing/style_guides.md) should be primarily done through
+ [automation](https://about.gitlab.com/handbook/values/#cleanup-over-sign-off)
+ instead of review comments.
- Asking the author to change the design sometimes means the complete rewrite
of the contributed code. It's usually a good idea to ask another maintainer or
reviewer before doing it, but have the courage to do it when you believe it is