Improve the contribution and MR review guide
Signed-off-by: Rémy Coutable <remy@rymai.me>
This commit is contained in:
parent
8581df3bfb
commit
2f7e28d1f7
|
@ -226,8 +226,7 @@ a feedback issue (if there isn't one already) and leave a comment asking for it
|
||||||
to be marked as `Accepting merge requests`. Please include screenshots or
|
to be marked as `Accepting merge requests`. Please include screenshots or
|
||||||
wireframes if the feature will also change the UI.
|
wireframes if the feature will also change the UI.
|
||||||
|
|
||||||
Merge requests can be filed either at [GitLab.com][gitlab-mr-tracker] or at
|
Merge requests should be opened at [GitLab.com][gitlab-mr-tracker].
|
||||||
[github.com][github-mr-tracker].
|
|
||||||
|
|
||||||
If you are new to GitLab development (or web development in general), see the
|
If you are new to GitLab development (or web development in general), see the
|
||||||
[I want to contribute!](#i-want-to-contribute) section to get you started with
|
[I want to contribute!](#i-want-to-contribute) section to get you started with
|
||||||
|
@ -246,10 +245,17 @@ tests are least likely to receive timely feedback. The workflow to make a merge
|
||||||
request is as follows:
|
request is as follows:
|
||||||
|
|
||||||
1. Fork the project into your personal space on GitLab.com
|
1. Fork the project into your personal space on GitLab.com
|
||||||
1. Create a feature branch, branch away from `master`.
|
1. Create a feature branch, branch away from `master`
|
||||||
1. Write [tests](https://gitlab.com/gitlab-org/gitlab-development-kit#running-the-tests) and code
|
1. Write [tests](https://gitlab.com/gitlab-org/gitlab-development-kit#running-the-tests) and code
|
||||||
1. Add your changes to the [CHANGELOG](CHANGELOG)
|
1. Add your changes to the [CHANGELOG](CHANGELOG):
|
||||||
1. If you are writing documentation, make sure to read the [documentation styleguide][doc-styleguide]
|
1. If you are fixing a ~regression issue, you can add your entry to the next
|
||||||
|
patch release (e.g. `8.12.5` if current version is `8.12.4`)
|
||||||
|
1. Otherwise, add your entry to the next minor release (e.g. `8.13.0` if
|
||||||
|
current version is `8.12.4`
|
||||||
|
1. Please add your entry at a random place among the entries of the targeted
|
||||||
|
release
|
||||||
|
1. If you are writing documentation, make sure to follow the
|
||||||
|
[documentation styleguide][doc-styleguide]
|
||||||
1. If you have multiple commits please combine them into one commit by
|
1. If you have multiple commits please combine them into one commit by
|
||||||
[squashing them][git-squash]
|
[squashing them][git-squash]
|
||||||
1. Push the commit(s) to your fork
|
1. Push the commit(s) to your fork
|
||||||
|
@ -258,7 +264,7 @@ request is as follows:
|
||||||
1. The MR description should give a motive for your change and the method you
|
1. The MR description should give a motive for your change and the method you
|
||||||
used to achieve it, see the [merge request description format]
|
used to achieve it, see the [merge request description format]
|
||||||
(#merge-request-description-format)
|
(#merge-request-description-format)
|
||||||
1. If the MR changes the UI it should include before and after screenshots
|
1. If the MR changes the UI it should include *Before* and *After* screenshots
|
||||||
1. If the MR changes CSS classes please include the list of affected pages,
|
1. If the MR changes CSS classes please include the list of affected pages,
|
||||||
`grep css-class ./app -R`
|
`grep css-class ./app -R`
|
||||||
1. Link any relevant [issues][ce-tracker] in the merge request description and
|
1. Link any relevant [issues][ce-tracker] in the merge request description and
|
||||||
|
@ -270,7 +276,9 @@ request is as follows:
|
||||||
[shell command guidelines](doc/development/shell_commands.md)
|
[shell command guidelines](doc/development/shell_commands.md)
|
||||||
1. If your code creates new files on disk please read the
|
1. If your code creates new files on disk please read the
|
||||||
[shared files guidelines](doc/development/shared_files.md).
|
[shared files guidelines](doc/development/shared_files.md).
|
||||||
1. When writing commit messages please follow [these](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html) [guidelines](http://chris.beams.io/posts/git-commit/).
|
1. When writing commit messages please follow
|
||||||
|
[these](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html)
|
||||||
|
[guidelines](http://chris.beams.io/posts/git-commit/).
|
||||||
1. If your merge request adds one or more migrations, make sure to execute all
|
1. If your merge request adds one or more migrations, make sure to execute all
|
||||||
migrations on a fresh database before the MR is reviewed. If the review leads
|
migrations on a fresh database before the MR is reviewed. If the review leads
|
||||||
to large changes in the MR, do this again once the review is complete.
|
to large changes in the MR, do this again once the review is complete.
|
||||||
|
@ -305,23 +313,6 @@ Please ensure that your merge request meets the contribution acceptance criteria
|
||||||
When having your code reviewed and when reviewing merge requests please take the
|
When having your code reviewed and when reviewing merge requests please take the
|
||||||
[code review guidelines](doc/development/code_review.md) into account.
|
[code review guidelines](doc/development/code_review.md) into account.
|
||||||
|
|
||||||
### Merge request description format
|
|
||||||
|
|
||||||
Please submit merge requests using the following template in the merge request
|
|
||||||
description area. Copy-paste it to retain the markdown format.
|
|
||||||
|
|
||||||
```
|
|
||||||
## What does this MR do?
|
|
||||||
|
|
||||||
## Are there points in the code the reviewer needs to double check?
|
|
||||||
|
|
||||||
## Why was this MR needed?
|
|
||||||
|
|
||||||
## What are the relevant issue numbers?
|
|
||||||
|
|
||||||
## Screenshots (if relevant)
|
|
||||||
```
|
|
||||||
|
|
||||||
### Contribution acceptance criteria
|
### Contribution acceptance criteria
|
||||||
|
|
||||||
1. The change is as small as possible
|
1. The change is as small as possible
|
||||||
|
@ -333,8 +324,8 @@ description area. Copy-paste it to retain the markdown format.
|
||||||
aforementioned failing test
|
aforementioned failing test
|
||||||
1. Your MR initially contains a single commit (please use `git rebase -i` to
|
1. Your MR initially contains a single commit (please use `git rebase -i` to
|
||||||
squash commits)
|
squash commits)
|
||||||
1. Your changes can merge without problems (if not please merge `master`, never
|
1. Your changes can merge without problems (if not please rebase if you're the
|
||||||
rebase commits pushed to the remote server)
|
only one working on your feature branch, otherwise, merge `master`)
|
||||||
1. Does not break any existing functionality
|
1. Does not break any existing functionality
|
||||||
1. Fixes one specific issue or implements one specific feature (do not combine
|
1. Fixes one specific issue or implements one specific feature (do not combine
|
||||||
things, send separate merge requests if needed)
|
things, send separate merge requests if needed)
|
||||||
|
@ -352,7 +343,10 @@ description area. Copy-paste it to retain the markdown format.
|
||||||
entire line to follow it. This prevents linting tools from generating warnings.
|
entire line to follow it. This prevents linting tools from generating warnings.
|
||||||
- Don't touch neighbouring lines. As an exception, automatic mass
|
- Don't touch neighbouring lines. As an exception, automatic mass
|
||||||
refactoring modifications may leave style non-compliant.
|
refactoring modifications may leave style non-compliant.
|
||||||
1. If the merge request adds any new libraries (gems, JavaScript libraries, etc.), they should conform to our [Licensing guidelines][license-finder-doc]. See the instructions in that document for help if your MR fails the "license-finder" test with a "Dependencies that need approval" error.
|
1. If the merge request adds any new libraries (gems, JavaScript libraries,
|
||||||
|
etc.), they should conform to our [Licensing guidelines][license-finder-doc].
|
||||||
|
See the instructions in that document for help if your MR fails the
|
||||||
|
"license-finder" test with a "Dependencies that need approval" error.
|
||||||
|
|
||||||
## Changes for Stable Releases
|
## Changes for Stable Releases
|
||||||
|
|
||||||
|
@ -468,7 +462,6 @@ available at [http://contributor-covenant.org/version/1/1/0/](http://contributor
|
||||||
[accepting-mrs-ce]: https://gitlab.com/gitlab-org/gitlab-ce/issues?label_name=Accepting+Merge+Requests
|
[accepting-mrs-ce]: https://gitlab.com/gitlab-org/gitlab-ce/issues?label_name=Accepting+Merge+Requests
|
||||||
[accepting-mrs-ee]: https://gitlab.com/gitlab-org/gitlab-ee/issues?label_name=Accepting+Merge+Requests
|
[accepting-mrs-ee]: https://gitlab.com/gitlab-org/gitlab-ee/issues?label_name=Accepting+Merge+Requests
|
||||||
[gitlab-mr-tracker]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests
|
[gitlab-mr-tracker]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests
|
||||||
[github-mr-tracker]: https://github.com/gitlabhq/gitlabhq/pulls
|
|
||||||
[gdk]: https://gitlab.com/gitlab-org/gitlab-development-kit
|
[gdk]: https://gitlab.com/gitlab-org/gitlab-development-kit
|
||||||
[git-squash]: https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits
|
[git-squash]: https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits
|
||||||
[closed-merge-requests]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests?assignee_id=&label_name=&milestone_id=&scope=&sort=&state=closed
|
[closed-merge-requests]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests?assignee_id=&label_name=&milestone_id=&scope=&sort=&state=closed
|
||||||
|
|
|
@ -34,6 +34,10 @@ request is up to one of our merge request "endbosses", denoted on the
|
||||||
|
|
||||||
## Having your code reviewed
|
## Having your code reviewed
|
||||||
|
|
||||||
|
Please keep in mind that code review is a process that can take multiple
|
||||||
|
iterations, and reviewer may spot things later that they may not have seen the
|
||||||
|
first time.
|
||||||
|
|
||||||
- The first reviewer of your code is _you_. Before you perform that first push
|
- The first reviewer of your code is _you_. Before you perform that first push
|
||||||
of your shiny new branch, read through the entire diff. Does it make sense?
|
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
|
Did you include something unrelated to the overall purpose of the changes? Did
|
||||||
|
@ -55,6 +59,7 @@ request is up to one of our merge request "endbosses", denoted on the
|
||||||
Understand why the change is necessary (fixes a bug, improves the user
|
Understand why the change is necessary (fixes a bug, improves the user
|
||||||
experience, refactors the existing code). Then:
|
experience, refactors the existing code). Then:
|
||||||
|
|
||||||
|
- Try to be thorough in your reviews to reduce the number of iterations.
|
||||||
- Communicate which ideas you feel strongly about and those you don't.
|
- Communicate which ideas you feel strongly about and those you don't.
|
||||||
- Identify ways to simplify the code while still solving the problem.
|
- Identify ways to simplify the code while still solving the problem.
|
||||||
- Offer alternative implementations, but assume the author already considered
|
- Offer alternative implementations, but assume the author already considered
|
||||||
|
@ -66,6 +71,8 @@ experience, refactors the existing code). Then:
|
||||||
"LGTM :thumbsup:", or "Just a couple things to address."
|
"LGTM :thumbsup:", or "Just a couple things to address."
|
||||||
- Avoid accepting a merge request before the build succeeds ("Merge when build
|
- Avoid accepting a merge request before the build succeeds ("Merge when build
|
||||||
succeeds" is fine).
|
succeeds" is fine).
|
||||||
|
- If you set the MR to "Merge when build succeeds", you should take over
|
||||||
|
subsequent revisions for anything that would be spotted after that.
|
||||||
|
|
||||||
## Credits
|
## Credits
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue