Add latest changes from gitlab-org/gitlab@13-11-stable-eev13.11.0-rc43

1 files changed, 65 insertions, 40 deletions

diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 0a811ceba65..731fec98933 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -38,14 +38,15 @@ Depending on the areas your merge request touches, it must be **approved** by on
 or more [maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#maintainer):
 
 For approvals, we use the approval functionality found in the merge request
-widget. Reviewers can add their approval by [approving additionally](../user/project/merge_requests/merge_request_approvals.md#adding-or-removing-an-approval).
+widget. For reviewers, we use the [reviewer functionality](../user/project/merge_requests/getting_started.md#reviewer) in the sidebar.
+Reviewers can add their approval by [approving additionally](../user/project/merge_requests/merge_request_approvals.md#adding-or-removing-an-approval).
 
 Getting your merge request **merged** also requires a maintainer. If it requires
 more than one approval, the last maintainer to review and approve merges it.
 
 ### Domain experts
 
-Domain experts are team members who have substantial experience with a specific technology, product feature or area of the codebase. Team members are encouraged to self-identify as domain experts and add it to their [team profile](https://gitlab.com/gitlab-com/www-gitlab-com/-/blob/master/data/team.yml)
+Domain experts are team members who have substantial experience with a specific technology, product feature or area of the codebase. Team members are encouraged to self-identify as domain experts and add it to their [team profile](https://gitlab.com/gitlab-com/www-gitlab-com/-/blob/master/data/team.yml).
 
 When self-identifying as a domain expert, it is recommended to assign the MR changing the `team.yml` to be merged by an already established Domain Expert or a corresponding Engineering Manager.
 
@@ -99,8 +100,9 @@ with [domain expertise](#domain-experts).
    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_maintainers_frontend)**.
-1. If your merge request includes UX changes (*1*), it must be
-   **approved by a [UX team member](https://about.gitlab.com/company/team/)**.
+1. If your merge request includes user-facing changes (*3*), it must be
+   **approved by a [Product Designer](https://about.gitlab.com/handbook/engineering/ux/product-design/)**,
+   based on assignments in the appropriate [DevOps stage group](https://about.gitlab.com/handbook/product/categories/#devops-stages).
 1. If your merge request includes adding a new JavaScript library (*1*)...
    - If the library significantly increases the
      [bundle size](https://gitlab.com/gitlab-org/frontend/playground/webpack-memory-metrics/-/blob/master/doc/report.md), it must
@@ -109,16 +111,14 @@ with [domain expertise](#domain-experts).
      GitLab, the license must be **approved by a [legal department member](https://about.gitlab.com/handbook/legal/)**.
      More information about license compatibility can be found in our
      [GitLab Licensing and Compatibility documentation](licensing.md).
-1. If your merge request includes adding a new UI/UX paradigm (*1*), it must be
-   **approved by a [UX lead](https://about.gitlab.com/company/team/)**.
 1. If your merge request includes a new dependency or a file system change, it must be
    **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/#assignments)**, based on
    the appropriate [product category](https://about.gitlab.com/handbook/product/categories/).
-1. If your merge request includes end-to-end **and** non-end-to-end changes (*3*), it must be **approved
+1. If your merge request includes end-to-end **and** non-end-to-end changes (*4*), it must be **approved
    by a [Software Engineer in Test](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors)**.
-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. If your merge request only includes end-to-end changes (*4*) **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. If your merge request includes a new or updated [application limit](https://about.gitlab.com/handbook/product/product-processes/#introducing-application-limits), it must be **approved by a [product manager](https://about.gitlab.com/company/team/)**.
 1. If your merge request includes Product Intelligence (telemetry or analytics) changes, it should be reviewed and approved by a [Product Intelligence engineer](https://gitlab.com/gitlab-org/growth/product_intelligence/engineers).
 1. If your merge request includes an addition of, or changes to a [Feature spec](testing_guide/testing_levels.md#frontend-feature-tests), it must be **approved by a [Quality maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_qa) or [Quality reviewer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_reviewers_qa)**.
@@ -128,7 +128,10 @@ with [domain expertise](#domain-experts).
 - (*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*): End-to-end changes include all files within the `qa` directory.
+- (*3*): User-facing changes include both visual changes (regardless of how minor),
+  and changes to the rendered DOM which impact how a screen reader may announce
+  the content.
+- (*4*): End-to-end changes include all files within the `qa` directory.
 
 #### Security requirements
 
@@ -137,9 +140,11 @@ View the updated documentation regarding [internal application security reviews]
 ### The responsibility of the merge request author
 
 The responsibility to find the best solution and implement it lies with the
-merge request author.
+merge request author. The author or [directly responsible individual](https://about.gitlab.com/handbook/people-group/directly-responsible-individuals/)
+will stay assigned to the merge request as the assignee throughout
+the code review lifecycle. If you are unable to set yourself as an assignee, ask a [reviewer](https://about.gitlab.com/handbook/engineering/workflow/code-review/#reviewer) to do this for you.
 
-Before assigning a merge request to a maintainer for approval and merge, they
+Before requesting a review from a maintainer to approve and merge, they
 should be confident that:
 
 - It actually solves the problem it was meant to solve.
@@ -184,12 +189,11 @@ Avoid:
   [include a link to the relevant issue](code_comments.md).
 - Adding comments which only explain what the code is doing. If non-TODO comments are added, they should
   [_explain why, not what_](https://blog.codinghorror.com/code-tells-you-how-comments-tell-you-why/).
-- Assigning merge requests with failed tests to maintainers. If the tests are failing and you have to assign, ensure you leave a comment with an explanation.
+- Requesting maintainer reviews of merge requests with failed tests. If the tests are failing and you have to request a review, ensure you leave a comment with an explanation.
 - Excessively mentioning maintainers through email or Slack (if the maintainer is reachable
-through Slack). If you can't assign a merge request, `@` mentioning a maintainer in a comment is acceptable and in all other cases assigning the merge request is sufficient.
+through Slack). If you can't add a reviewer for a merge request, `@` mentioning a maintainer in a comment is acceptable and in all other cases adding a reviewer is sufficient.
 
-This
-[saves reviewers time and helps authors catch mistakes earlier](https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/index.html#__RefHeading__97_174136755).
+This saves reviewers time and helps authors catch mistakes earlier.
 
 ### The responsibility of the reviewer
 
@@ -197,9 +201,10 @@ This
 that it meets all requirements, you should:
 
 - Click the Approve button.
-- Advise the author their merge request has been reviewed and approved.
-- Assign the merge request to a maintainer. Default to assigning it to a maintainer with [domain expertise](#domain-experts),
+- `@` mention the author to generate a to-do notification, and advise them that their merge request has been reviewed and approved.
+- Request a review from a maintainer. Default to requests for a maintainer with [domain expertise](#domain-experts),
 however, if one isn't available or you think the merge request doesn't need a review by a [domain expert](#domain-experts), feel free to follow the [Reviewer roulette](#reviewer-roulette) suggestion.
+- Remove yourself as a reviewer.
 
 ### The responsibility of the maintainer
 
@@ -227,7 +232,7 @@ If a developer who happens to also be a maintainer was involved in a merge reque
 as a reviewer, it is recommended that they are not also picked as the maintainer to ultimately approve and merge it.
 
 Maintainers should check before merging if the merge request is approved by the
-required approvers.
+required approvers. If still awaiting further approvals from others, remove yourself as a reviewer then `@` mention the author and explain why in a comment. Stay as reviewer if you're merging the code.
 
 Maintainers must check before merging if the merge request is introducing new
 vulnerabilities, by inspecting the list in the Merge Request
@@ -245,6 +250,19 @@ Note that certain Merge Requests may target a stable branch. These are rare
 events. These types of Merge Requests cannot be merged by the Maintainer.
 Instead these should be sent to the [Release Manager](https://about.gitlab.com/community/release-managers/).
 
+After merging, a maintainer should stay as the reviewer listed on the merge request.
+
+### Dogfooding the Reviewers feature
+
+On March 18th 2021, an updated process was put in place aimed at efficiently and consistently dogfooding the Reviewers feature.
+
+Here is a summary of the changes, also reflected in this section above.
+
+- Merge request authors and DRIs stay as Assignees
+- Authors request a review from Reviewers when they are expected to review
+- Reviewers remove themselves after they're done reviewing/approving
+- The last approver stays as Reviewer upon merging
+
 ## Best practices
 
 ### Everyone
@@ -304,11 +322,11 @@ first time.
 - 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
   able to read individual updates based on their earlier feedback.
-- Assign the merge request back to the reviewer once you are ready for another round of
-  review. If you do not have the ability to assign merge requests, `@`
+- Request a new review from the reviewer once you are ready for another round of
+  review. If you do not have the ability to request a review, `@`
   mention the reviewer instead.
 
-### Assigning a merge request for a review
+### Requesting a review
 
 When you are ready to have your merge request reviewed,
 you should request an initial review by assigning it to a reviewer from your group or team.
@@ -319,14 +337,14 @@ You can also use `workflow::ready for review` label. That means that your merge
 When your merge request receives an approval from the first reviewer it can be passed to a maintainer. You should default to choosing a maintainer with [domain expertise](#domain-experts), and otherwise follow the Reviewer Roulette recommendation or use the label `ready for merge`.
 
 Sometimes, a maintainer may not be available for review. They could be out of the office or [at capacity](#review-response-slo).
-You can and should check the maintainer’s availability in their profile. If the maintainer recommended by
+You can and should check the maintainer's availability in their profile. If the maintainer recommended by
 the roulette is not available, choose someone else from that list.
 
-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.
+It is the responsibility of the author for the merge request to be reviewed. If it stays in the `ready for review` state too long it is recommended to request a review from a specific reviewer.
 
 #### 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?state=opened&label_name%5B%5D=workflow%3A%3Aready%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 add themselves as a reviewer for any merge request they want to review.
 
 ### Reviewing a merge request
 
@@ -347,12 +365,11 @@ experience, refactors the existing code). Then:
   - For non-mandatory suggestions, decorate with (non-blocking) so the author knows they can
     optionally resolve within the merge request or follow-up at a later stage.
   - There's a [Chrome/Firefox add-on](https://gitlab.com/conventionalcomments/conventional-comments-button) which you can use to apply [Conventional Comment](https://conventionalcomments.org/) prefixes.
-- Ensure there are no open dependencies. Check [related issues](../user/project/issues/related_issues.md) for blockers. Clarify with the author(s)
+- Ensure there are no open dependencies. Check [linked issues](../user/project/issues/related_issues.md) for blockers. Clarify with the author(s)
 if necessary. If blocked by one or more open MRs, set an [MR dependency](../user/project/merge_requests/merge_request_dependencies.md).
 - After a round of line notes, it can be helpful to post a summary note such as
   "Looks good to me", or "Just a couple things to address."
-- Assign the merge request to the author if changes are required following your
-  review.
+- Let the author know if changes are required following your review.
 
 ### Merging a merge request
 
@@ -371,8 +388,7 @@ those changes directly without going back to the author. You can do this by
 using the [suggest changes](../user/discussions/index.md#suggest-changes) feature to apply
 your own suggestions to the merge request. Note that:
 
-- If the changes are not straightforward, please prefer assigning the merge request back
-  to the author.
+- If the changes are not straightforward, please prefer allowing the author to make the change.
 - **Before applying suggestions**, edit the merge request to make sure
   [squash and
   merge](../user/project/merge_requests/squash_and_merge.md#squash-and-merge)
@@ -392,17 +408,26 @@ When ready to merge:
   circling back with the author about that. Otherwise, if the MR only has a few commits, we'll
   be respecting the author's setting by not squashing them.
 
-- **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:
+WARNING:
+**If the merge request is from a fork, review all changes thoroughly for malicious code before
+starting a [Pipeline for Merged Results](../ci/merge_request_pipelines/index.md#run-pipelines-in-the-parent-project-for-merge-requests-from-a-forked-project).**
+Pay particular attention to new dependencies and dependency updates, such as Ruby gems and Node packages.
+While changes to files like `Gemfile.lock` or `yarn.lock` might appear trivial, they could lead to the
+fetching of malicious packages.
+If the MR source branch is more than 100 commits behind the target branch, ask the author to rebase it.
+Review links and images, especially in documentation MRs.
+When in doubt, ask someone from `@gitlab-com/gl-security/appsec` to review the merge request **before starting any merge request pipeline**.
+
+- 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 **[master is broken](https://about.gitlab.com/handbook/engineering/workflow/#broken-master),
-    do not merge the merge request**. Follow these specific [handbook instructions](https://about.gitlab.com/handbook/engineering/workflow/#maintaining-throughput-during-broken-master).
+    do not merge the merge request** except for
+    [very specific cases](https://about.gitlab.com/handbook/engineering/workflow/#criteria-for-merging-during-broken-master).
+    For other cases, follow these [handbook instructions](https://about.gitlab.com/handbook/engineering/workflow/#merging-during-broken-master).
   - If the **latest [Pipeline for Merged Results](../ci/merge_request_pipelines/pipelines_for_merged_results/#pipelines-for-merged-results)** 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**, we can use [Pipelines for Merged Results from a forked project](../ci/merge_request_pipelines/index.md#run-pipelines-in-the-parent-project-for-merge-requests-from-a-forked-project) with caution.
-    Before triggering the pipeline, review all changes for **malicious code**.
-    If you cannot trigger the pipeline, review the status of the fork relative to `master`.
-    If it's more than 100 commits behind, ask the author to rebase it before merging.
 - When you set the MR to "Merge When Pipeline Succeeds", you should take over
   subsequent revisions for anything that would be spotted after that.
 
@@ -500,7 +525,7 @@ Enterprise Edition instance. This has some implications:
 ### Review turnaround time
 
 Because [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,
+reviewers are expected to review 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
@@ -515,7 +540,7 @@ To ensure swift feedback to ready-to-review code, we maintain a `Review-response
 If you don't think you can review a merge request in 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 is able to, so that they can be unblocked
-and get on with their work quickly.
+and get on with their work quickly. Remove yourself as a reviewer.
 
 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
@@ -529,7 +554,7 @@ 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
+the `Review-response` SLO, they are free to remind the reviewer through Slack or add
 another reviewer.
 
 ### Customer critical merge requests
@@ -539,7 +564,7 @@ A merge request may benefit from being considered a customer critical priority b
 Properties of customer critical merge requests:
 
 - The [Senior Director of Development](https://about.gitlab.com/job-families/engineering/engineering-management/#senior-director-engineering) ([@clefelhocz1](https://gitlab.com/clefelhocz1)) is the DRI for deciding if a merge request is customer critical.
-- The DRI assigns the `customer-critical-merge-request` label to the merge request.
+- The DRI applies the `customer-critical-merge-request` label to the merge request.
 - It is required that the reviewer(s) and maintainer(s) involved with a customer critical merge request are engaged as soon as this decision is made.
 - It is required to prioritize work for those involved on a customer critical merge request so that they have the time available necessary to focus on it.
 - It is required to adhere to GitLab [values](https://about.gitlab.com/handbook/values/) and processes when working on customer critical merge requests, taking particular note of family and friends first/work second, definition of done, iteration, and release when it's ready.