Skip to content
Snippets Groups Projects
Commit 895f2092 authored by Jérémy Bobbio (Lunar)'s avatar Jérémy Bobbio (Lunar)
Browse files

Update “Code review” for GitLab

Now we have switched to GitLab, code review is done via merge requests.
Update the process to the new terminology and provided tools.

Let’s also move the `patch-submission` ref from the obsolete Phabricator
page to the one about GitLab.

Related to #4732
parent 8b396dc3
No related branches found
No related tags found
No related merge requests found
.. _code-review:
Code Review
Code review
===========
This page documents code review practices used for Software Heritage development.
`GitLab has many useful features
<https://docs.gitlab.com/ee/user/project/merge_requests/reviews/>`_ to ease
reviewing the changes, communicating around questions or comments, and making
suggestions. Learning how to use them will help you and the team.
Guidelines
----------
Please adhere to the following guidelines to perform and obtain code reviews
(CRs) in the context of Software Heritage development:
1. **CRs are strongly recommended** for any non-trivial code change,
but not mandatory (nor enforced at the VCS level).
2. The CR :ref:`workflow <patch-submission>` is implemented using
Phabricator/Differential.
3. Explicitly **suggest reviewer(s)** when submitting new CR requests:
either the most knowledgeable person(s) for the target code or the general
`reviewers <https://forge.softwareheritage.org/project/view/50/>`_
(which is the `default <https://forge.softwareheritage.org/H18>`_).
4. **Review anything you want**: no matter the suggested reviewer(s),
feel free to review any outstanding CR.
5. **One LGTM is enough**: feel free to approve any outstanding CR.
6. **Review every day**: CRs should be timely as fellow developers
will wait for them.
To make CRs sustainable each developer should strive to dedicate
a fixed minimum amount of CR time every (work) day.
For more detailed suggestions (and much more) on the motivational
and practical aspects of code reviews see Good reads below.
Good reads
----------
Please adhere to the following guidelines in the context of Software Heritage
development.
When submitting changes:
- **Reviews are strongly recommended** for any non-trivial code change,
but not mandatory (nor enforced at Git level).
- The :ref:`code submission workflow <patch-submission>` is implemented using
merge requests sent to our GitLab instance.
- The `assignee
<https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#assignee>`_
in the context of merge request is the person supposed to drive it to
completion. **In almost all cases, you should assign yourself to merge requests you
create.**
- If your changes are meant to address a specific issue, you should `prefix the
branch name with the issue number
<https://docs.gitlab.com/ee/user/project/repository/branches/index.html#naming>`_.
- Feel free to explicitly **mention one or more specific
reviewer** from people most knowledgeable with the target code. You can also
assign a single person [#multiple-reviewers]_ as reviewer using the relevant
field. It will prevent the merge request from going unnoticed.
- **One approval is enough** before merging a request.
As a team member:
- **Review any merge requests you want**: no matter the suggested reviewers,
feel free to review any pending merge requests.
- **Review every day**: reviews should be timely as fellow developers
will wait for them. To make the process sustainable each developer should
strive to dedicate a fixed minimum amount of review time every workday.
.. [#multiple-reviewers] Assigning multiple reviewers are only supported in
paid editions of GitLab. As we currently use the free *Community Edition*,
mentions are the only way to get the attention of multiple people at once.
Learning about pending reviews
------------------------------
As we aim to review merge requests in a timely manner, here are several ways to
know about merge requests waiting for input.
Notifications
^^^^^^^^^^^^^
To be sure to receive notifications of new merge requests:
1. Open the `notification controls
<https://gitlab.softwareheritage.org/-/profile/notifications>`_ for your
GitLab account.
2. Locate the *Platform* group.
3. Select *Custom* in the list of notification levels.
4. Click on the bell icon.
5. Make sure *New merge request* is ticked. You probably also want to select at
least *New issue*, *Failed pipeline*, and *Fixed pipeline*.
6. Click *Ok*.
7. Repeat the operation for other groups of interest.
List pending merge requests
^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sadly, notifications can easily be missed and GitLab by itself does not provide
a view with all merge requests waiting for action across multiple projects. To
have a better view of pending merge requests, you should consider using:
- `GitLab Notify Extension
<https://github.com/Mikescops/gitlab-notify-extension>`_ available for Chrome
and Firefox. It can list all merge requests you are assigned to and all merge
requests you created.
- `gitlab-revq <https://gitlab.softwareheritage.org/vlorentz/gitlab-revq/>`_, a
command-line tool listing all actionable pending requests.
Good reads on various angles of code review:
Recommended readings
--------------------
* `Best practices (Palantir) <https://medium.com/palantir/code-review-best-practices-19e02780015f>`_ ← comprehensive and recommended read, especially if you're short on time
* `Best practices (Thoughtbot) <https://github.com/thoughtbot/guides/tree/master/code-review>`_
......
......@@ -32,6 +32,8 @@ documentation. Please follow the steps below to setup your account.
- Sign the Software Heritage Contributor License Agreement. Please
contact us to know more about this.
.. _patch-submission:
Development workflow
--------------------
......
......@@ -15,8 +15,6 @@ https://gitlab.softwareheritage.org/
The content below is no longer relevant and will be updated soon.
.. _patch-submission:
Submitting patches
==================
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment