|
| 1 | +# PR Guidelines |
| 2 | + |
| 3 | +This document outlines the steps that need to be taken before merging any PR. In most cases, the only required action is creating a changelog entry (see below). However, when a PR affects the database schema, or the API, or service configuration, extra steps are required, and those are detailed below. |
| 4 | + |
| 5 | +The recommended way to use this document is to copy the relevant checklists below into the PR description, when appropriate, and make sure they are all checked before the PR is merged. |
| 6 | + |
| 7 | +## Changelog entries |
| 8 | + |
| 9 | +Every PR should add a new file in the appropriate subdirectory of `changelog.d`, containing just the text of the corresponding changelog entry. There is no need to explicitly write a PR number, because the `mk-changelog.sh` script (used on release) will add it automatically at the end. The name of the file does not matter, but it should be unique to avoid unnecessary conflicts (e.g. use the branch name). |
| 10 | + |
| 11 | +It is still possible to write the PR number manually if so desired, which is useful in case the entry is shared by multiple PRs, or if the PR is merged with a merge commit rather than by squashing. In that case, the script would leave the PR number reference intact, as long as it is at the very end of the entry, with no period afterwards, in brackets, and preceded by a `#` symbol (e.g. #2646). |
| 12 | + |
| 13 | +As long as the PR is merged by squashing, it is also possible to use the pattern `##` to refer to the current PR number. This will be replaced throughout. |
| 14 | + |
| 15 | +Multiline entries are supported, and are handled correctly by the script. Again, the PR reference should either be omitted or put at the very end. If multiple entries for a single PR are desired, there should be a different file for each of them. |
| 16 | + |
| 17 | +See `docs/legacy/developer/changelog.md` for more information. |
| 18 | + |
| 19 | +## Schema migrations |
| 20 | + |
| 21 | +If a cassandra schema migration has been added then |
| 22 | + |
| 23 | + - [ ] Run **`make git-add-cassandra-schema`** to update the cassandra schema documentation |
| 24 | + |
| 25 | +### Incompatible schema migrations and data migrations |
| 26 | + |
| 27 | +If the PR contains a cassandra *schema* migration which is backwards incompatible, a changelog entry should be added to the release notes. See [notes on Cassandra](https://github.com/wireapp/wire-server/blob/develop/docs/developer/cassandra-interaction.md#cassandra-schema-migrations) for more details on how to implement such schema changes. A similar entry should be added if the PR contains a *data* migration. |
| 28 | + |
| 29 | + - [ ] Add a changelog entry in `0-release-notes` detailing measures to be taken by instance operators |
| 30 | + |
| 31 | +## Adding new public endpoints |
| 32 | + |
| 33 | +When adding new endpoints in the Haskell code in wire-server, correct routing needs to be applied at the nginz level. |
| 34 | + |
| 35 | +NB: The nginz paths are interpreted as *prefixes*. If you add a new end-point that is identical to an existing one except for the path of the latter being a proper prefix of the former, and if the nginz configuration of the two paths should be the same, nothing needs to be done. Exception: if you see a path like `/self$`, you know it doesn't match `/self/sub/what`. |
| 36 | + |
| 37 | +The following needs to be done, as part of a PR adding endpoints or changing endpoint paths. |
| 38 | + |
| 39 | + - [ ] Update nginz config in helm: `charts/nginz/values.yaml` |
| 40 | + - [ ] Update nginz config in the demo: `deploy/services-demo/conf/nginz/nginx.conf` |
| 41 | + |
| 42 | +### Helm configuration |
| 43 | + |
| 44 | +For internal endpoints for QA access on staging environments, copy a block with `/i/` containing |
| 45 | +``` |
| 46 | +zauth: false |
| 47 | +basic_auth: true |
| 48 | +whitelisted_envs: ['staging'] |
| 49 | +``` |
| 50 | + |
| 51 | +For customer support access to an internal endpoint, instead update code in [stern](https://github.com/wireapp/wire-server/tree/develop/tools/stern) as part of your PR. There is no need to add that endpoint to nginz. |
| 52 | + |
| 53 | +### Demo nginz configuration |
| 54 | + |
| 55 | +New entris should include `common_response_no_zauth.conf;` for public endpoints without authentication and `common_response_with_zauth.conf;` for regular (authenticated) endpoints. Browse the file to see examples. |
| 56 | + |
| 57 | +### Example |
| 58 | + |
| 59 | +If the following endpoints are added to galley: |
| 60 | + |
| 61 | +``` |
| 62 | +GET /new/endpoint |
| 63 | +POST /turtles |
| 64 | +PUT /turtles/<turtleId>/name |
| 65 | +``` |
| 66 | + |
| 67 | +Add to `charts/nginz/values.yaml`, under the `galley` section: |
| 68 | + |
| 69 | +``` |
| 70 | +- path: /new/endpoint |
| 71 | +- path: ^/turtles(.*) |
| 72 | +``` |
| 73 | + |
| 74 | +## Adding new configuration flags in wire-server |
| 75 | + |
| 76 | +If a PR adds new configuration options for say brig, the following files need to be edited: |
| 77 | + |
| 78 | +* [ ] The parser under `services/brig/src/Brig/Options.hs` |
| 79 | +* [ ] The integration test config: `services/brig/brig.integration.yaml` |
| 80 | +* [ ] The demo config: `deploy/services-demo/conf/brig.demo.yaml` and `deploy/services-demo/conf/brig.demo.yaml` |
| 81 | +* [ ] The charts: `charts/brig/templates/configmap.yaml` |
| 82 | +* [ ] The default values: `charts/brig/values.yaml` |
| 83 | +* [ ] The values files for CI: `hack/helm_vars/wire-server/values.yaml` |
| 84 | +* [ ] The configuration docs: `docs/legacy/reference/config-options.md` |
| 85 | + |
| 86 | +If any new configuration value is required and has no default, then: |
| 87 | + |
| 88 | +* [ ] Write a changelog entry in `0-release-notes` advertising the new configuration value |
| 89 | +* [ ] Update all the relevant environments |
| 90 | + |
| 91 | +For wire Cloud, look into all the relevant environments (look for `helm_vars/wire-server/values.yaml.gotmpl` files in cailleach). Ideally, these configuration updates should be merged **before** merging the corresponding wire-server PR. |
| 92 | + |
| 93 | +### Removing configuration flags |
| 94 | + |
| 95 | +Remove them with the PR from wire-server `./charts` folder, as charts are linked to code and go hand-in hand. Possibly notify all operators (through `./changelog.d/0-release-notes/`) that some overrides may not have any effect anymore. |
| 96 | + |
| 97 | +### Renaming configuration flags |
| 98 | + |
| 99 | +Avoid doing this. If you must, see Removing/adding sections above. But please note that all people who have an installation of wire also may have overridden any of the configuration option you may wish to change the name of. As this is not type checked, it's very error prone and people may find themselves with default configuration values being used instead of their intended configuration settings. Guideline: only rename for good reasons, not for aesthetics; or be prepared to spend a significant |
| 100 | +amount on documenting and communication about this change. |
| 101 | + |
| 102 | +## Changes to developer workflow |
| 103 | + |
| 104 | +If a PR changes development workflow or dependencies, they should be automated and documented under `docs/developer/`. All efforts should be taken to minimize development setup breakage or slowdown for co-workers. |
0 commit comments