| jrperritt | 9b7b9e6 | 2016-07-11 22:30:50 -0500 | [diff] [blame^] | 1 | |
| 2 | ## On Pull Requests |
| 3 | |
| 4 | - Before you start a PR there needs to be a Github issue and a discussion about it |
| 5 | on that issue with a core contributor, even if it's just a 'SGTM'. |
| 6 | |
| 7 | - A PR's description must reference the issue it closes with a `For <ISSUE NUMBER>` (e.g. For #293). |
| 8 | |
| 9 | - A PR's description must contain link(s) to the line(s) in the OpenStack |
| 10 | source code (on Github) that prove(s) the PR code to be valid. Links to documentation |
| 11 | are not good enough. The link(s) should be to a non-`master` branch. For example, |
| 12 | a pull request implementing the creation of a Neutron v2 subnet might put the |
| 13 | following link in the description: |
| 14 | https://github.com/openstack/neutron/blob/stable/mitaka/neutron/api/v2/attributes.py#L749 |
| 15 | From that link, a reviewer (or user) can verify the fields in the request/response |
| 16 | objects in the PR. |
| 17 | |
| 18 | - A PR that is in-progress should have `[wip]` in front of the PR's title. When |
| 19 | ready for review, remove the `[wip]` and ping a core contributor with an `@`. |
| 20 | |
| 21 | - A PR should be small. Even if you intend on implementing an entire |
| 22 | service, a PR should only be one route of that service |
| 23 | (e.g. create server or get server, but not both). |
| 24 | |
| 25 | - Unless explicitly asked, do not squash commits in the middle of a review; only |
| 26 | append. It makes it difficult for the reviewer to see what's changed from one |
| 27 | review to the next. |
| 28 | |
| 29 | ## On Code |
| 30 | |
| 31 | - In re design: follow as closely as is reasonable the code already in the library. |
| 32 | Most operations (e.g. create, delete) admit the same design. |
| 33 | |
| 34 | - Unit tests and acceptance (integration) tests must be written to cover each PR. |
| 35 | Tests for operations with several options (e.g. list, create) should include all |
| 36 | the options in the tests. This will allow users to verify an operation on their |
| 37 | own infrastructure and see an example of usage. |
| 38 | |
| 39 | - If in doubt, ask in-line on the PR. |