|
|
> :warning: :hammer_and_wrench: I think we can revise this page completely. We now use merge requests in a way that is much closer to what Gitlab intends.
|
|
|
> —nickm, 14 August 2020
|
|
|
|
|
|
# NetworkTeam's Gitlab Code Review Workflow
|
|
|
|
|
|
This is how the network team uses Gitlab to do code reviews.
|
|
|
|
|
|
### Marking something for review
|
|
|
|
|
|
When you write code that you need reviewed, you create a Gitlab Merge Request and a Github Pull Request (for CI to work until we get GL CI). Within that MR, you reference the GH PR.
|
|
|
When you write code that you need reviewed, you create a Gitlab Merge Request.
|
|
|
|
|
|
You should also make your branch against the target maint-0.x.y branch, and your MR request against that target branch. Finally, if the MR is a backport candidate you should mention it in your MR message.
|
|
|
You should also make your branch against the target maint-0.x.y branch, and
|
|
|
your MR request against that target branch. Finally, if the MR is a backport
|
|
|
candidate you should mention it in your MR message.
|
|
|
|
|
|
### Assigning reviews
|
|
|
|
|
|
When asn and David assign reviews for little-t-tor we check for unassigned GL Merge requests in the components of little-t-tor, chutney and sbws. When we want to assign a merge request to Alice, we change the "Assigned" field of the MR to Alice.
|
|
|
|
|
|
Here are the relevant queries we use:
|
|
|
|
|
|
- little-t-tor: https://gitlab.torproject.org/tpo/core/tor/-/merge_requests?scope=all&utf8=%E2%9C%93&state=opened¬[label_name][]=Backport
|
|
|
|
|
|
- torspec: https://gitlab.torproject.org/tpo/core/torspec/-/merge_requests
|
|
|
|
|
|
- chutney: https://gitlab.torproject.org/tpo/core/chutney/-/merge_requests
|
|
|
|
|
|
- sbws: https://gitlab.torproject.org/tpo/network-health/sbws/-/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&assignee_id=None
|
|
|
|
|
|
### Finding your reviews
|
|
|
|
|
|
To find your reviews you use this query:
|
|
|
|
|
|
ahf:
|
|
|
https://gitlab.torproject.org/groups/tpo/-/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&assignee_username=ahf
|
|
|
|
|
|
nickm:
|
|
|
https://gitlab.torproject.org/groups/tpo/-/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&assignee_username=nickm
|
|
|
|
|
|
asn:
|
|
|
https://gitlab.torproject.org/groups/tpo/-/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&assignee_username=asn
|
|
|
Our `triage-bot` will automatically assign a member of the network team as a
|
|
|
reviewer.
|
|
|
|
|
|
dgoulet:
|
|
|
https://gitlab.torproject.org/groups/tpo/-/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&assignee_username=dgoulet
|
|
|
### Reviewing
|
|
|
|
|
|
[
|
|
|
Also until we finish reviewing all the old tickets (from the pre-MR era) please also check the following links:
|
|
|
If you are happy, use the `Approve` button.
|
|
|
|
|
|
ahf:
|
|
|
https://gitlab.torproject.org/tpo/core/tor/-/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Needs%20Review&assignee_username=ahf
|
|
|
|
|
|
nickm:
|
|
|
https://gitlab.torproject.org/tpo/core/tor/-/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Needs%20Review&assignee_username=nickm
|
|
|
|
|
|
asn:
|
|
|
https://gitlab.torproject.org/tpo/core/tor/-/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Needs%20Review&assignee_username=asn
|
|
|
|
|
|
dgoulet:
|
|
|
https://gitlab.torproject.org/tpo/core/tor/-/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Needs%20Review&assignee_username=dgoulet
|
|
|
]
|
|
|
|
|
|
|
|
|
### Doing reviews
|
|
|
|
|
|
#### Reviewing your reviews (happy path)
|
|
|
|
|
|
When you do your reviews, the happy path is that the patch was good and
|
|
|
you merge it, close the MR and the gitlab issue.
|
|
|
If it needs revision, put in the ~Needs Revision label (optional).
|
|
|
|
|
|
If the branch needs to get **backported**:
|
|
|
1. Give the MR the `Backport` label
|
|
|
2. Un-assign yourself from the MR: you don't need to review it any more.
|
|
|
3. Put the MR into the latest milestone to which it has not been backported.
|
|
|
|
|
|
#### Reviewing your reviews (sad path)
|
|
|
|
|
|
If the patch you are reviewing needs revisions, you need to change the Assignee of the MR back to the patch author. This signals to the patch author that revisions need to be done (plus also adds it into their TODO list)
|
|
|
|
|
|
### Revising your branches
|
|
|
|
|
|
If a branch needs to get revised, the author revises it, pushes the revised branch, and switches the Assignee field back to the reviewier (effectively also adding it to the reviewer's TODO list). |
|
|
\ No newline at end of file |
|
|
2. Put the MR into the latest milestone to which it has not been backported. |