Commit d743dc09 authored by intrigeri's avatar intrigeri
Browse files

GitLab: document how to propose, review, and merge changes

In passing, this commit starts making the submit/review/merge doc less focused
on code changes. This needs more work.
parent 2196f3fc
[[!meta title="Git merge policy"]] [[!meta title="Proposing, reviewing, and merging changes"]]
<div id="intro"> <div id="intro">
<p>Tails is developed in a set of [[Git repositories|contribute/git]].</p> <p>Tails is developed in [[contribute/working_together/GitLab]],
using a number of [[Git repositories|contribute/git]].</p>
[[!toc levels=2]] [[!toc levels=2]]
...@@ -14,10 +15,10 @@ ...@@ -14,10 +15,10 @@
<a id="submit"></a> <a id="submit"></a>
# How to submit your changes # How to submit your proposed changes
[[!inline pages="contribute/merge_policy/submit" raw=yes sort="age"]] [[!inline pages="contribute/merge_policy/submit" raw=yes sort="age"]]
# Review and merge process # How to review and merge proposed changes
[[!inline pages="contribute/merge_policy/review" raw=yes sort="age"]] [[!inline pages="contribute/merge_policy/review" raw=yes sort="age"]]
[[!meta title="Git merge policy: review and merge process"]] [[!meta title="How to review and merge proposed changes"]]
<a id="review"></a> <a id="review"></a>
## Review ## Review
- When you start doing a review'n'merge, assign the relevant ticket - Assign the [[!tails_gitlab help/user/project/merge_requests desc="Merge
and merge request to you, in order to avoid duplicated work. Request"]] to you. This avoids duplicating work.
- Merge the base branch (the one you're supposed to merge the reviewed - Verify that the destination branch matches what's in `config/base_branch` on
topic branch into, as specified in `config/base_branch` and in the the branch, and is correct:
pull request -- they must match) into the topic branch. - 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.
- Build the topic branch and test the feature. - Build the topic branch and test the feature.
- Check automated ISO build and test results on - Check automated ISO build and test results on
<https://jenkins.tails.boum.org/>. <https://jenkins.tails.boum.org/>.
- Check the diff e.g. with `git log -p`. Beware of changes introduced - Review the diff, either in the GitLab interface or on the command line.
If you're using the command line, beware of changes introduced
by merge commits: either add the `--cc` option to `git log`, or use by merge commits: either add the `--cc` option to `git log`, or use
`git diff` after reviewing the individual patches. `git diff` after reviewing the individual patches.
- Check the contents of every APT overlay that the branch enables: - Check the contents of every APT overlay that the branch enables:
...@@ -29,7 +35,11 @@ ...@@ -29,7 +35,11 @@
- <https://deb.tails.boum.org/dists/${APT_overlay}/main/binary-amd64/Packages> - <https://deb.tails.boum.org/dists/${APT_overlay}/main/binary-amd64/Packages>
- Check the user and design documentation. - Check the user and design documentation.
- Check the ticket. - 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>
- Changes proposed by new contributors, or by the patch'n'forget kind, - Changes proposed by new contributors, or by the patch'n'forget kind,
shall respectively be applied once they are in *good enough* state. shall respectively be applied once they are in *good enough* state.
That is, once the core developers team feels like maintaining it on That is, once the core developers team feels like maintaining it on
...@@ -41,50 +51,33 @@ ...@@ -41,50 +51,33 @@
proposed branch to avoid round-trips: for example, fixing typos proposed branch to avoid round-trips: for example, fixing typos
and improving phrasing of comments and strings. and improving phrasing of comments and strings.
Then, report back about these changes on the ticket. Then, report back about these changes on the ticket.
- Remember that it's hard to receive negative feedback. Don't forget
to note the good parts, be constructive and precise in your ## Give feedback
comments, and never use reviews to make personal attacks. You can
read these blog posts on review and 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:
- [Kate Heddleston: Criticism and ineffective feedback](https://www.kateheddleston.com/blog/criticism-and-ineffective-feedback) (blog post on how to do a good review) - [Kate Heddleston: Criticism and ineffective feedback](https://www.kateheddleston.com/blog/criticism-and-ineffective-feedback) (blog post on how to do a good review)
- [Liane Davey: Maybe you shouldn't give feedback](http://www.3coze.com/2016/07/17/bite-your-tongue/) - [Liane Davey: Maybe you shouldn't give feedback](http://www.3coze.com/2016/07/17/bite-your-tongue/)
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*.
<a id="merge"></a> <a id="merge"></a>
## Merge ## Merge
<div class="note"> Click the *Merge* button on the MR.
If the proposed change was submitted as a merge request on
[[!tails_gitlab desc="our GitLab"]], don't If you are not authorized to do so, instead:
click <i>Merge</i> there: while we can use GitLab for reviews, our
infrastructure is not ready to consume merge operations done there, so - Add a comment on the MR to say you have reviewed the branch.
you need to merge on the command line. If you approve its merging, say so.
</div> - De-assign yourself from the MR.
Merge the branch with `--no-ff`:
- 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`
<a id="closes"></a>
<a id="fix-committed"></a>
Please consider including `Closes: #NNNN` in the commit
message, _NNNN_ being the ticket number that is fixed by the branch
you are merging. Then, Redmine will automatically close the
corresponding ticket once you push the results of
your merge to our main [[Git repository|contribute/Git]]. For example:
Merge branch 'bugfix/8470-clean-up-apt-pinning' (Closes: #8470)
## Book keeping
1. Update the *Status* field on the ticket. If there is no remaining
tasks listed on the ticket, then change its status to *Resolved*
(unless you used the `Closes` keyword documented
above); else, set it back to *In Progress* and ask the branch submitter
to split the remaining tasks into other tickets.
1. Push the updated branch to the master Git repository.
1. Reply to the email that requested the review, if any.
[[!meta title="Git merge policy: how to submit code"]] [[!meta title="How to propose changes"]]
All development should happen in topic branches. For a new feature XXX, it should We use [[!tails_gitlab desc="GitLab"]] for tracking issues, merge requests, and
be named `feature/XXX`. For a bugfix about YYY, it should be named code. Please ensure you are familiar with [[how we use GitLab at
`bugfix/YYY`. Ideally, include the relevant ticket number in the topic Tails|contribute/working_together/GitLab]].
branch name, e.g. `bugfix/7173-upgrade-syslinux`.
All proposed changes should be prepared in topic branches. For a new feature XXX
When you think it is good enough and have tested your branch with its current tracked by issue number NNNN, it should be named `feature/NNNN-XXX`.
target branch (`config/base_branch`) merged into it: For a bugfix about YYY, it should be named `bugfix/NNNN-YYY`.
1. Push your branch To submit your branch for review:
- If you have commit access to the official Tails Git repository,
push your branch there so our CI picks it up. 1. Test your branch. If your branch contains code changes, test it with its
- Else, push to your personal Git repository: target branch (`config/base_branch`) merged into it.
fork us on [[!tails_gitweb_repo tails/tails desc="our GitLab"]].
2. Set the ticket's *Status* field to *In Progress* (if you do not see 2. Push your branch
this field when editing the ticket, ask the [[sysadmin team|contribute/working_together/roles/sysadmins]]
to grant you the necessary permissions). If you have commit access to the official Tails Git repository,
3. Point the ticket's *Feature Branch* field either to your branch push your branch there so our CI picks it up.
or to a merge request.
4. Set the ticket's *Target version* field to the release you would Else, push to your own fork of the Tails
like your changes to be in. [[!tails_gitweb_repo tails/tails desc="Git repository"]].
5. Make it clear what you're requesting: merging? some advice? an initial
code review of work that's not finished yet? 3. Once you would like your branch to be reviewed, and possibly merged,
6. If you have access to our Jenkins instance and you are requesting a merge: submit it:
- Ensure your branch builds on Jenkins.
- Either report about the test suite scenarios you've seen pass 1. If your branch contains code changes and you have access to our Jenkins
successfully locally, or check that the test suite passes instance:
on Jenkins.
7. Set the ticket's *Status* field to *Needs Validation*. - Ensure your branch builds on Jenkins.
8. Set the ticket's *Assignee* field appropriately: - Either take note of the test suite scenarios you've seen pass
- If it's already obvious to you who can and should review your branch: successfully locally, or check that the test suite passes on Jenkins.
assign the ticket to this person.
- Else, assign the ticket to nobody, i.e. unassign it from yourself. 2. On the corresponding GitLab issue:
The [[Foundations Team|working_together/roles/foundations_team]]
will either handle the review themselves or help you find a suitable - Set the *Milestone* field to the release you would like your changes to
reviewer. land in.
8. For important changes, if you feel the need to ask input from the - If there's a *To Do* or *Doing* label, replace it with the *Needs
greater development community, notify the [[tails-dev@boum.org|about/contact#tails-dev]] Validation* label.
mailing list.
3. Create a [[!tails_gitlab help/user/project/merge_requests desc="Merge
Then, if the [[reviewer|contribute/merge_policy/review]] asks for more Request"]] (aka. MR). There are [[!tails_gitlab
development to be done before help/user/project/merge_requests/creating_merge_requests.html desc="many
merging, they should set the ticket's *Status* field back to *In Progress*; ways"]] to do so. For example, you can click the *Create merge request*
from now on it's the responsibility of the branch/ticket "holder" to button on the corresponding issue.
change it back to *Needs Validation* once they consider the issues raised by
the reviewer are fixed. In this new MR:
The reviewer is allowed to commit trivial fixes on top of the - Use the description to:
proposed branch to avoid round-trips: for example, fixing typos - Summarize what problem this MR will fix, in terms of impact on users.
and improving phrasing of comments and strings. They must - Reference the issues this MR will solve: `Closes #xxx, #yyyy`.
report back about these changes on the ticket. - If it's obvious to you who can, and should, review your branch: assign
the MR to that person.
Else, leave the MR unassigned:
the [[Foundations Team|working_together/roles/foundations_team]] will
either handle the review themselves or help you find a suitable reviewer.
- If you have run some test suite scenarios locally: report about your
results in a new comment.
Then, for every subsequent submit/review/fix cycle: once you've fixed all
problems raised by the reviewer, update the issue and MR metadata again as
documented above (milestone, assignee, and labels).
...@@ -49,10 +49,7 @@ Some people are case sensitive, please try to consider that. ...@@ -49,10 +49,7 @@ Some people are case sensitive, please try to consider that.
### Open issues ### Open issues
Each open issue must have exactly 0 or 1 of these labels: Each open issue can have up to 1 status label:
- No such label: newly created issue that needs to be triaged
by the UX team and Foundations Team.
- _To Do_: it would be good if someone worked on this issue - _To Do_: it would be good if someone worked on this issue
...@@ -61,6 +58,13 @@ Each open issue must have exactly 0 or 1 of these labels: ...@@ -61,6 +58,13 @@ Each open issue must have exactly 0 or 1 of these labels:
- _Needs Validation_: a resolution has been proposed and needs to be reviewed. - _Needs Validation_: a resolution has been proposed and needs to be reviewed.
For details, see our [[merge policy|/contribute/merge_policy]]. For details, see our [[merge policy|/contribute/merge_policy]].
The main advantage of using these labels is to organize and visualize issues on
[[!tails_gitlab help/user/project/issue_board.html desc="Issue Boards"]].
Some teams use a workflow that depends on accurately using these labels.
Apart of that, you may consider these labels as optional.
### Closed issues ### Closed issues
Closing an issue means one of: Closing an issue means one of:
...@@ -264,10 +268,21 @@ It is important to: ...@@ -264,10 +268,21 @@ It is important to:
If you don't think you will manage to work on an issue any time soon, If you don't think you will manage to work on an issue any time soon,
it's often best to make this clear on the issue, or to de-assign yourself. it's often best to make this clear on the issue, or to de-assign yourself.
For details, see how we use the [[assignee|GitLab#assignee]] information. For details, see how we use the [[assignee|GitLab#assignee]]
and [[milestone|GitLab#milestone]] information.
# How to propose, review, and merge changes
We use [[!tails_gitlab help/user/project/merge_requests desc="Merge Requests"]]
(aka. MRs) to propose, review, and merge changes.
See the [[how to propose, review, and merge
changes|contribute/merge_policy]] documentation.
# How to request and provide input # How to request and provide input
<a id="discussions"></a>
## How to participate in discussions ## How to participate in discussions
You can comment on issues and merge requests. You can comment on issues and merge requests.
...@@ -278,6 +293,9 @@ _Comment_: a thread can be marked as resolved, while a comment cannot. ...@@ -278,6 +293,9 @@ _Comment_: a thread can be marked as resolved, while a comment cannot.
This helps keeping track of which discussions have reached a conclusion, This helps keeping track of which discussions have reached a conclusion,
and which ones are still pending. and which ones are still pending.
For more information, see the GitLab documentation about [[!tails_gitlab
help/user/discussions/index.md desc="Discussions"]].
## Requesting input from someone else ## Requesting input from someone else
If you need input from someone else on an issue or merge request, If you need input from someone else on an issue or merge request,
...@@ -305,10 +323,6 @@ immediately, you're responsible for keeping track of this task, for example via ...@@ -305,10 +323,6 @@ immediately, you're responsible for keeping track of this task, for example via
the To-Do list, or by creating a new issue assigned to yourself, or using the To-Do list, or by creating a new issue assigned to yourself, or using
whatever personal organization tools work for you. whatever personal organization tools work for you.
# How to submit and review merge requests
See the [[contribute/merge_policy]].
# Email interface # Email interface
Using the email address registered with your GitLab account, Using the email address registered with your GitLab account,
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment