| 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: |
| jrperritt | 792d51f | 2016-07-18 11:48:55 -0500 | [diff] [blame^] | 14 | |
| jrperritt | 9b7b9e6 | 2016-07-11 22:30:50 -0500 | [diff] [blame] | 15 | https://github.com/openstack/neutron/blob/stable/mitaka/neutron/api/v2/attributes.py#L749 |
| jrperritt | 792d51f | 2016-07-18 11:48:55 -0500 | [diff] [blame^] | 16 | |
| jrperritt | 9b7b9e6 | 2016-07-11 22:30:50 -0500 | [diff] [blame] | 17 | From that link, a reviewer (or user) can verify the fields in the request/response |
| 18 | objects in the PR. |
| 19 | |
| 20 | - A PR that is in-progress should have `[wip]` in front of the PR's title. When |
| 21 | ready for review, remove the `[wip]` and ping a core contributor with an `@`. |
| 22 | |
| 23 | - A PR should be small. Even if you intend on implementing an entire |
| 24 | service, a PR should only be one route of that service |
| 25 | (e.g. create server or get server, but not both). |
| 26 | |
| 27 | - Unless explicitly asked, do not squash commits in the middle of a review; only |
| 28 | append. It makes it difficult for the reviewer to see what's changed from one |
| 29 | review to the next. |
| 30 | |
| 31 | ## On Code |
| 32 | |
| 33 | - In re design: follow as closely as is reasonable the code already in the library. |
| 34 | Most operations (e.g. create, delete) admit the same design. |
| 35 | |
| 36 | - Unit tests and acceptance (integration) tests must be written to cover each PR. |
| 37 | Tests for operations with several options (e.g. list, create) should include all |
| 38 | the options in the tests. This will allow users to verify an operation on their |
| 39 | own infrastructure and see an example of usage. |
| 40 | |
| 41 | - If in doubt, ask in-line on the PR. |
| jrperritt | 792d51f | 2016-07-18 11:48:55 -0500 | [diff] [blame^] | 42 | |
| 43 | ### File Structure |
| 44 | |
| 45 | - The following should be used in most cases: |
| 46 | |
| 47 | - `requests.go`: contains all the functions that make HTTP requests and the |
| 48 | types associated with the HTTP request (parameters for URL, body, etc) |
| 49 | - `results.go`: contains all the response objects and their methods |
| 50 | - `urls.go`: contains the endpoints to which the requests are made |
| 51 | |
| 52 | ### Naming |
| 53 | |
| 54 | - For methods on a type in `response.go`, the receiver should be named `r` and the |
| 55 | variable into which it will be unmarshalled `s`. |
| 56 | |
| 57 | - Functions in `requests.go`, with the exception of functions that return a |
| 58 | `pagination.Pager`, should be named returns of the name `r`. |
| 59 | |
| 60 | - Functions in `requests.go` that accept request bodies should accept as their |
| 61 | last parameter an `interface` named `<Action>OptsBuilder` (eg `CreateOptsBuilder`). |
| 62 | This `interface` should have at the least a method named `To<Resource><Action>Map` |
| 63 | (eg `ToPortCreateMap`). |
| 64 | |
| 65 | - Functions in `requests.go` that accept query strings should accept as their |
| 66 | last parameter an `interface` named `<Action>OptsBuilder` (eg `ListOptsBuilder`). |
| 67 | This `interface` should have at the least a method named `To<Resource><Action>Query` |
| 68 | (eg `ToServerListQuery`). |