From a020cde93079cf3ea305cb89c66109051c7f4a40 Mon Sep 17 00:00:00 2001 From: Kathy Wang Date: Mon, 26 Nov 2018 19:07:34 +0000 Subject: Update code_review.md --- doc/development/code_review.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index fc3ffcd2c6b..85d0247cd8e 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -51,9 +51,9 @@ from other teams than your own. ### Security requirements - 1. If your merge request is processing, storing, or transferring any kind of [RED data][red data], possibly orange data too, it must be + 1. If your merge request is processing, storing, or transferring any kind of [RED data][red data], and/or ORANGE data, it must be **approved by a [Security Engineer][team]**. - 1. If your merge request is implementing, utilizing, or related to any type of authentication, authorization, or session handling mechanism, it must be + 1. If your merge request involves implementing, utilizing, or is otherwise related to any type of authentication, authorization, or session handling mechanism, it must be **approved by a [Security Engineer][team]**. 1. If your merge request has a goal which requires a cryptographic function such as: confidentiality, integrity, authentication, or non-repudiation, it must be **approved by a [Security Engineer][team]**. @@ -98,8 +98,8 @@ Since a maintainer's job only depends on their knowledge of the overall GitLab codebase, and not that of any specific domain, they can review, approve and merge merge requests from any team and in any product area. -In fact, authors are recommended to get their merge requests merged by maintainers -from other teams than their own, to ensure that all code across GitLab is consistent +In fact, authors are encouraged to get their merge requests merged by maintainers +from teams other than their own, to ensure that all code across GitLab is consistent and can be easily understood by all contributors, from both inside and outside the company, without requiring team-specific expertise. @@ -109,7 +109,7 @@ placed to do so without an unreasonable investment of time. In those cases, they will defer to the judgment of the author and earlier reviewers and involved domain experts, in favor of focusing on their primary responsibilities. -If a developer who happens to also be a maintainer was involved in a merge request +If a developer happens to also be a maintainer, and was involved in a merge request 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. @@ -118,14 +118,14 @@ required approvers. Maintainers must check before merging if the merge request is introducing new vulnerabilities, by inspecting the list in the Merge Request [Security -Widget][security-widget]. In doubt, a [Security Engineer][team] can be +Widget][security-widget]. When in doubt, a [Security Engineer][team] can be involved. The list of detected vulnerabilities must be either empty or containing: - dismissed vulnerabilities in case of false positives - vulnerabilities converted to issues Maintainers should **never** dismiss vulnerabilities to "empty" the list, -without duly verify them. +without duly verifying them. ## Best practices @@ -177,7 +177,7 @@ first time. ### Assigning a merge request for a review -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. +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. -- cgit v1.2.1