review.mdwn 3.61 KB
Newer Older
1
[[!meta title="How to review and merge proposed changes"]]
Tails developers's avatar
Tails developers committed
2

intrigeri's avatar
intrigeri committed
3
4
5
6
7
8
9
We use [[!tails_gitlab desc="GitLab"]] for tracking issues, merge requests, and
code. Please ensure you are familiar with [[how we use GitLab at
Tails|contribute/working_together/GitLab]].

In particular, see
[[how to participate in discussions|working_together/GitLab#discussions]].

intrigeri's avatar
intrigeri committed
10
11
<a id="review"></a>

12
## Review
Tails developers's avatar
Tails developers committed
13

14
15
16
17
18
19
20
21
22
23
- Assign the [[!tails_gitlab help/user/project/merge_requests desc="Merge
  Request"]] to you. This avoids duplicating work.
- Verify that the destination branch matches what's in `config/base_branch` on
  the branch, and is correct:
  - for a new feature: into `devel`
  - for a fix on top on the last RC: into `testing`; then merge
    `testing` into `devel`
  - for a fix on top of the last stable: into `stable`; then merge
    `stable` into `devel`
- Merge the destination branch of the MR into the topic branch.
24
- Build the topic branch and test the feature.
25
26
- Check automated ISO build and test results on
  <https://jenkins.tails.boum.org/>.
27
28
- Review the diff, either in the GitLab interface or on the command line.
  If you're using the command line, beware of changes introduced
intrigeri's avatar
intrigeri committed
29
  by merge commits: either add the `--cc` option to `git log`, or use
30
  `git diff` after reviewing the individual patches.
31
32
33
34
35
36
37
38
39
40
41
42
43
- Check the contents of every APT overlay that the branch enables:

  - If you have upload rights to our [[custom APT
    repository|contribute/APT_repository/custom]]:

        ssh reprepro@incoming.deb.tails.boum.org \
            reprepro list ${APT_overlay}`

  - Else:

    - <https://deb.tails.boum.org/dists/${APT_overlay}/main/source/Sources>
    - <https://deb.tails.boum.org/dists/${APT_overlay}/main/binary-amd64/Packages>

44
- Check the user and design documentation.
45
46
47
48
49
- Check the corresponding GitLab issue.
- Ensure the description of the MR includes `Closes #NNNN`
  statements for the issues fixed by this MR.
  <a id="closes"></a>
  <a id="fix-committed"></a>
50
51
52
53
54
55
56
- Changes proposed by new contributors, or by the patch'n'forget kind,
  shall respectively be applied once they are in *good enough* state.
  That is, once the core developers team feels like maintaining it on
  the long run in case the initial submitter disappears. Such
  maintenance includes: polishing the proposed changes to make them fit
  for release; writing and updating the design and end-user
  documentations; fixing bugs.
intrigeri's avatar
intrigeri committed
57
58
59
60
- As reviewer, you are allowed to commit trivial fixes on top of the
  proposed branch to avoid round-trips: for example, fixing typos
  and improving phrasing of comments and strings.
  Then, report back about these changes on the ticket.
61
62
63
64
65
66
67
68

## Give feedback

First, remember that it's hard to receive negative feedback. Don't forget
to note the good parts, be constructive and precise in your
comments, and never use reviews to make personal attacks. You can
read these blog posts on review and feedback:

69
  - [Kate Heddleston: Criticism and ineffective feedback](https://www.kateheddleston.com/blog/criticism-and-ineffective-feedback) (blog post on how to do a good review)
sajolida's avatar
sajolida committed
70
  - [Liane Davey: Maybe you shouldn't give feedback](http://www.3coze.com/2016/07/17/bite-your-tongue/)
Tails developers's avatar
Tails developers committed
71

72
73
74
75
76
77
78
79
As part of your review, if you spot problems that block the merge
in your opinion:

1. Give feedback about these problems.
2. Reassign the MR to the person who proposed the branch.
3. On the corresponding issue: replace the *Needs Validation* label
   with *Doing*.

intrigeri's avatar
intrigeri committed
80
81
<a id="merge"></a>

82
## Merge
Tails developers's avatar
Tails developers committed
83

84
85
86
87
88
89
90
Click the *Merge* button on the MR.

If you are not authorized to do so, instead:

 - Add a comment on the MR to say you have reviewed the branch.
   If you approve its merging, say so.
 - De-assign yourself from the MR.