|
| 1 | +# Issues |
| 2 | + |
| 3 | +We welcome reports of technical issues with the components of the xen-api |
| 4 | +toolstack. Please make sure that the description of the issue is as detailed as |
| 5 | +possible to help anyone investigating it: |
| 6 | + |
| 7 | +1) Mention how it was detected, if and how it could be reproduced |
| 8 | + |
| 9 | +1) What's the desired behaviour? In what cases would it be useful? |
| 10 | + |
| 11 | +1) Include error messages, related logs if appropriate |
| 12 | + |
| 13 | +# Pull Requests |
| 14 | + |
| 15 | +To contribute changes to xen-api, please fork the repository on |
| 16 | +GitHub, and then submit a pull request. |
| 17 | + |
| 18 | +It is required to add a `Signed-off-by:` as a |
| 19 | +[Developers Certificate of Origin](http://developercertificate.org). |
| 20 | +It certifies the patch's origin and is licensed under an |
| 21 | +appropriate open-source licence to include it in Xapi: |
| 22 | +https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---signoff |
| 23 | + |
| 24 | +The following points are intended to describe what makes a contribution "good" - |
| 25 | +easier to review, integrate, and maintain. Please follow them in your work. |
| 26 | + |
| 27 | +## Commit subjects and PR titles |
| 28 | + |
| 29 | +Commit subjects should preferrably start with the name of the component the |
| 30 | +commit is most related to, and describe what the commit achieves. If your |
| 31 | +commit only touches the `ocaml/xenopsd` directory, it should look like this, |
| 32 | +for example: |
| 33 | + |
| 34 | +``` |
| 35 | +xenopsd: Fix a deadlock during VM suspend |
| 36 | +``` |
| 37 | + |
| 38 | +Similar principle applies to Pull Request titles. If there is only a single |
| 39 | +commit in the PR, Github will automatically copy its subject and description to |
| 40 | +the PR's title and body. If there are several commits in the PR, describe what |
| 41 | +the PR achieves and the components it most directly impacts. |
| 42 | + |
| 43 | +If the commit subject includes some tracking identifier (such as `CP-1234`, for |
| 44 | +example) referring to internal systems, please make sure to include all of the |
| 45 | +essential information in the public descriptions - describe the symptoms of the |
| 46 | +issue, how it was detected, investigated, how it could be reproduced, what are |
| 47 | +the trade-offs and so on as appropriate. |
| 48 | + |
| 49 | +## Split into commits |
| 50 | + |
| 51 | +Following from the rules described above, if what the commit achieves is |
| 52 | +difficult to fit into its subject, it is probably better to split it into |
| 53 | +several commits, if possible. Note that every commit should build (`make` |
| 54 | +should work and the CI should pass) independently, without requiring future |
| 55 | +commits. This means some modifications can't really be split into several |
| 56 | +commits (datamodel changes, in particular, require modifications to several |
| 57 | +components at the same time), but makes it easier to revert part of the Pull |
| 58 | +Request if some issues are detected in integration testing at a later point. |
| 59 | + |
| 60 | +## Good Commit Messages |
| 61 | + |
| 62 | +Commit messages (and the body of a Pull Request) should be as helpful and |
| 63 | +descriptive as possible. If applicable, please include a description of current |
| 64 | +behaviour, your changes, and the new behaviour. Justify the reasoning behind |
| 65 | +your changes - are they sufficient on their own, or preparing for more changes? |
| 66 | +Link any appropriate documentation, issues, or commits. |
| 67 | + |
| 68 | +## CI |
| 69 | + |
| 70 | +Please make sure your Pull Request passes the Github CI. It will verify that |
| 71 | +your code has been properly formatted (can be done locally with `make format`), |
| 72 | +builds (`make` and `make check`), and passes the unit tests (`make test`). |
| 73 | +The CI will run in the branches of your fork, so you can verify it passes |
| 74 | +there before opening a Pull Request. |
| 75 | + |
| 76 | +## Testing |
| 77 | + |
| 78 | +Describe what kind of testing your contribution underwent. If the testing was |
| 79 | +manual, please describe the commands or external clients that were used. If the |
| 80 | +tests were automated, include at least a cursory description/name of the tests, |
| 81 | +when they were regressed, if possible. |
| 82 | + |
| 83 | +Please note that any contribution to the code of the project will likely |
| 84 | +require at least some testing to be done. Depending on how central the |
| 85 | +component touched in your PR is to the system, the more things could only be |
| 86 | +detected in real-world usecases through integration testing. |
| 87 | + |
| 88 | +If a commit has been determined to break integration testing at a later stage, |
| 89 | +please note that the first and safest measure will almost always be reverting |
| 90 | +the faulty commit. Making sure critical tests are passing remains a priority |
| 91 | +over waiting for some commit to be reworked or refactored (which can be worked |
| 92 | +on after a revert has been done). Though we are striving to make more tests |
| 93 | +public (with failure then being visible to all), as long as some critical tests |
| 94 | +remain private, this will also apply to such tests (with maintainers flagging |
| 95 | +the breakage preferrably describing at least the gist of the test). |
| 96 | + |
| 97 | +If you are still waiting on some testing to be done, please mark the PR as a |
| 98 | +"draft" and make the reasoning clear. |
| 99 | + |
| 100 | +If your contribution doesn't intend to have any functional changes, please make |
| 101 | +that clear as well. |
| 102 | + |
| 103 | +## Feature work |
| 104 | + |
| 105 | +If your contribution adds some new feature or reworks some major aspect of the |
| 106 | +system (as opposed to one-off fixes), it can be benefitial to first describe |
| 107 | +the plan of your work in a design proposal. Architectural issues are better |
| 108 | +spotted early on, and taking a big-picture view can often lead to new insights. |
| 109 | + |
| 110 | +An example of a design proposal is here: |
| 111 | + |
| 112 | +https://github.com/xapi-project/xen-api/pull/6387 |
| 113 | + |
| 114 | +If submitting a design first is not possible, include documentation alongside |
| 115 | +with your PR describing the work, like it was done in the last three commits |
| 116 | +here: |
| 117 | + |
| 118 | +https://github.com/xapi-project/xen-api/pull/6457 |
| 119 | + |
| 120 | +Note that the design will often serve as documentation as well - so take care |
| 121 | +updating it after the implementation is done to better reflect reality. |
| 122 | + |
| 123 | +## Review process and merge |
| 124 | + |
| 125 | +It can often be useful to address review suggestions with a "fixup" commit |
| 126 | +(created manually or with the help of `git commit --fixup=HASH`). This way it |
| 127 | +is clear what the original code was and what your fix touches. Once the |
| 128 | +fixup commit has been reviewed and the PR approved, please squash the fixup |
| 129 | +commits with `git rebase --autosquash` before merging. Otherwise the commits in |
| 130 | +the Pull Request should stay as independent commits - we do not require |
| 131 | +squashing all the commits into a single one on merge. |
0 commit comments