Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2021-05-19 18:44:42 +0300
committerGitLab Bot <gitlab-bot@gitlab.com>2021-05-19 18:44:42 +0300
commit4555e1b21c365ed8303ffb7a3325d773c9b8bf31 (patch)
tree5423a1c7516cffe36384133ade12572cf709398d /doc/development/code_review.md
parente570267f2f6b326480d284e0164a6464ba4081bc (diff)
Add latest changes from gitlab-org/gitlab@13-12-stable-eev13.12.0-rc42
Diffstat (limited to 'doc/development/code_review.md')
-rw-r--r--doc/development/code_review.md81
1 files changed, 61 insertions, 20 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 731fec98933..b91e27b7051 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -39,7 +39,7 @@ or more [maintainers](https://about.gitlab.com/handbook/engineering/workflow/cod
For approvals, we use the approval functionality found in the merge request
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).
+Reviewers can add their approval by [approving additionally](../user/project/merge_requests/approvals/index.md#approve-a-merge-request).
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.
@@ -101,7 +101,7 @@ with [domain expertise](#domain-experts).
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 user-facing changes (*3*), it must be
- **approved by a [Product Designer](https://about.gitlab.com/handbook/engineering/ux/product-design/)**,
+ **approved by a [Product Designer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_reviewers_UX)**,
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
@@ -114,8 +114,8 @@ with [domain expertise](#domain-experts).
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/).
+ by a [Technical writer](https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments)**,
+ based on assignments in the appropriate [DevOps stage group](https://about.gitlab.com/handbook/product/categories/#devops-stages).
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 (*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)**
@@ -299,8 +299,10 @@ first time.
of your shiny new branch, read through the entire diff. Does it make sense?
Did you include something unrelated to the overall purpose of the changes? Did
you forget to remove any debugging code?
-- Consider providing instructions on how to test the merge request. This can be
- helpful for reviewers not familiar with the product feature or area of the codebase.
+- Write a detailed description as outlined in the [merge request guidelines](contributing/merge_request_workflow.md#merge-request-guidelines).
+ Some reviewers may not be familiar with the product feature or area of the
+ codebase. Thorough descriptions help all reviewers understand your request
+ and test effectively.
- If you know your change depends on another being merged first, note it in the
description and set an [merge request dependency](../user/project/merge_requests/merge_request_dependencies.md).
- Be grateful for the reviewer's suggestions. (`Good call. I'll make that change.`)
@@ -371,6 +373,9 @@ if necessary. If blocked by one or more open MRs, set an [MR dependency](../user
"Looks good to me", or "Just a couple things to address."
- Let the author know if changes are required following your review.
+WARNING:
+**If the merge request is from a fork, also check the [additional guidelines for community contributions](#community-contributions).**
+
### Merging a merge request
Before taking the decision to merge:
@@ -385,7 +390,7 @@ If a merge request is fundamentally ready, but needs only trivial fixes (such as
typos), consider demonstrating a [bias for
action](https://about.gitlab.com/handbook/values/#bias-for-action) by making
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
+using the [suggest changes](../user/project/merge_requests/reviews/suggestions.md) feature to apply
your own suggestions to the merge request. Note that:
- If the changes are not straightforward, please prefer allowing the author to make the change.
@@ -399,6 +404,9 @@ your own suggestions to the merge request. Note that:
When ready to merge:
+WARNING:
+**If the merge request is from a fork, also check the [additional guidelines for community contributions](#community-contributions).**
+
- Consider using the [Squash and
merge](../user/project/merge_requests/squash_and_merge.md#squash-and-merge)
feature when the merge request has a lot of commits.
@@ -407,17 +415,6 @@ When ready to merge:
messy commit history, it will be more efficient to squash commits instead of
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.
-
-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:
@@ -430,6 +427,10 @@ When in doubt, ask someone from `@gitlab-com/gl-security/appsec` to review the m
enough to `master`.
- When you set the MR to "Merge When Pipeline Succeeds", you should take over
subsequent revisions for anything that would be spotted after that.
+- For merge requests that have had [Squash and
+ merge](../user/project/merge_requests/squash_and_merge.md#squash-and-merge) set,
+ the squashed commit’s default commit message is taken from the merge request title.
+ You're encouraged to [select a commit with a more informative commit message](../user/project/merge_requests/squash_and_merge.md#overview) before merging.
Thanks to **Pipeline for Merged Results**, authors no longer have to rebase their
branch as frequently anymore (only when there are conflicts) because the Merge
@@ -439,6 +440,45 @@ for a final rebase: instead, they only have to start a MR pipeline and set MWPS.
This step brings us very close to the actual Merge Trains feature by testing the
Merge Results against the latest `master` at the time of the pipeline creation.
+### Community contributions
+
+WARNING:
+**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).**
+
+When reviewing merge requests added by wider community contributors:
+
+- 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.
+- 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 manually starting any merge request pipeline**.
+
+If the MR source branch is more than 1,000 commits behind the target branch:
+
+- Ask the author to rebase it, or consider taking a bias-for-action and rebasing it yourself
+ if the MR has "Allows commits from members who can merge to the target branch" enabled.
+- Reviewing MRs in the context of recent changes can help prevent hidden runtime conflicts and
+ promote consistency. Depending on the nature of the change, you might also want to rebase if the
+ MR is less than 1,000 commits behind.
+- A forced push could throw off the contributor, so it's a good idea to communicate that you've performed a rebase,
+ or check with the contributor first when they're actively working on the MR.
+- The rebase can usually be done inside GitLab with the `/rebase` [quick action](../user/project/quick_actions.md).
+
+When an MR needs further changes but the author is not responding for a long period of time,
+or unable to finish the MR, we can take it over in accordance with our
+[Closing policy for issues and merge requests](contributing/#closing-policy-for-issues-and-merge-requests):
+
+1. Add a comment to their MR saying you'll take it over to be able to get it merged.
+1. Add the label `~"coach will finish"` to their MR.
+1. Create a new feature branch from the main branch.
+1. Merge their branch into your new feature branch.
+1. Open a new merge request to merge your feature branch into the main branch.
+1. Link the community MR from your MR and label it as `~"Community contribution"`.
+1. Make any necessary final adjustments and ping the contributor to give them the chance to review your changes, and to make them aware that their content is being merged into the main branch.
+1. Make sure the content complies with all the merge request guidelines.
+1. Follow the regular review process as we do for any merge request.
+
### The right balance
One of the most difficult things during code review is finding the right
@@ -519,8 +559,9 @@ Enterprise Edition instance. This has some implications:
1. Try to avoid that, and add to `ApplicationSetting` instead.
1. Ensure that it is also
[added to Omnibus](https://docs.gitlab.com/omnibus/settings/gitlab.yml.html#adding-a-new-setting-to-gitlab-yml).
-1. **File system access** can be slow, so try to avoid
- [shared files](shared_files.md) when an alternative solution is available.
+1. **File system access** is not possible in a [cloud-native architecture](architecture.md#adapting-existing-and-introducing-new-components).
+ Ensure that we support object storage for any file storage we need to perform. For more
+ information, see the [uploads documentation](uploads.md).
### Review turnaround time