]>
Commit | Line | Data |
---|---|---|
7c673cae FG |
1 | ========================== |
2 | Submitting Patches to Ceph | |
3 | ========================== | |
4 | ||
9f95a23c | 5 | Patches to Ceph can be divided into three categories: |
7c673cae | 6 | |
9f95a23c TL |
7 | 1. patches targeting Ceph kernel code |
8 | 2. patches targeting the "master" branch | |
9 | 3. patches targeting stable branches (e.g.: "nautilus") | |
7c673cae | 10 | |
9f95a23c TL |
11 | Some parts of Ceph - notably the RBD and CephFS kernel clients - are maintained |
12 | within the Linux Kernel. For patches targeting this code, please refer to the | |
13 | file ``SubmittingPatches-kernel.rst``. | |
7c673cae | 14 | |
9f95a23c TL |
15 | The rest of this document assumes that your patch relates to Ceph code that is |
16 | maintained in the GitHub repository https://github.com/ceph/ceph | |
7c673cae | 17 | |
9f95a23c TL |
18 | If you have a patch that fixes an issue, feel free to open a GitHub pull request |
19 | ("PR") targeting the "master" branch, but do read this document first, as it | |
20 | contains important information for ensuring that your PR passes code review | |
21 | smoothly. | |
7c673cae | 22 | |
9f95a23c TL |
23 | For patches targeting stable branches (e.g. "nautilus"), please also see |
24 | the file ``SubmittingPatches-backports.rst``. | |
25 | ||
26 | .. contents:: | |
27 | :depth: 3 | |
7c673cae | 28 | |
7c673cae | 29 | |
9f95a23c TL |
30 | Sign your work |
31 | -------------- | |
7c673cae FG |
32 | |
33 | The sign-off is a simple line at the end of the explanation for the | |
9f95a23c | 34 | commit, which certifies that you wrote it or otherwise have the right to |
11fdf7f2 | 35 | pass it on as a open-source patch. The rules are pretty simple: if you |
7c673cae FG |
36 | can certify the below: |
37 | ||
38 | Developer's Certificate of Origin 1.1 | |
39 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
40 | ||
41 | By making a contribution to this project, I certify that: | |
42 | ||
43 | (a) The contribution was created in whole or in part by me and I | |
44 | have the right to submit it under the open source license | |
45 | indicated in the file; or | |
46 | ||
47 | (b) The contribution is based upon previous work that, to the best | |
48 | of my knowledge, is covered under an appropriate open source | |
49 | license and I have the right under that license to submit that | |
50 | work with modifications, whether created in whole or in part | |
51 | by me, under the same open source license (unless I am | |
52 | permitted to submit under a different license), as indicated | |
53 | in the file; or | |
54 | ||
55 | (c) The contribution was provided directly to me by some other | |
56 | person who certified (a), (b) or (c) and I have not modified | |
57 | it. | |
58 | ||
59 | (d) I understand and agree that this project and the contribution | |
60 | are public and that a record of the contribution (including all | |
61 | personal information I submit with it, including my sign-off) is | |
62 | maintained indefinitely and may be redistributed consistent with | |
63 | this project or the open source license(s) involved. | |
64 | ||
65 | then you just add a line saying :: | |
66 | ||
67 | Signed-off-by: Random J Developer <random@developer.example.org> | |
68 | ||
f67539c2 | 69 | using your real name (sorry, no pseudonyms or anonymous contributions). |
7c673cae | 70 | |
9f95a23c TL |
71 | Git can sign off on your behalf |
72 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
7c673cae | 73 | |
9f95a23c TL |
74 | Please note that git makes it trivially easy to sign commits. First, set the |
75 | following config options:: | |
7c673cae | 76 | |
9f95a23c TL |
77 | $ git config --list | grep user |
78 | user.email=my_real_email_address@example.com | |
79 | user.name=My Real Name | |
7c673cae | 80 | |
9f95a23c TL |
81 | Then just remember to use ``git commit -s``. Git will add the ``Signed-off-by`` |
82 | line automatically. | |
7c673cae | 83 | |
7c673cae | 84 | |
9f95a23c TL |
85 | Separate your changes |
86 | --------------------- | |
7c673cae | 87 | |
9f95a23c | 88 | Group *logical changes* into individual commits. |
7c673cae | 89 | |
9f95a23c TL |
90 | If you have a series of bulleted modifications, consider separating each of |
91 | those into its own commit. | |
7c673cae | 92 | |
9f95a23c TL |
93 | For example, if your changes include both bug fixes and performance enhancements |
94 | for a single component, separate those changes into two or more commits. If your | |
95 | changes include an API update, and a new feature which uses that new API, | |
96 | separate those into two patches. | |
7c673cae | 97 | |
9f95a23c TL |
98 | On the other hand, if you make a single change that affects numerous |
99 | files, group those changes into a single commit. Thus a single logical change is | |
100 | contained within a single patch. (If the change needs to be backported, that | |
101 | might change the calculus, because smaller commits are easier to backport.) | |
7c673cae | 102 | |
7c673cae | 103 | |
9f95a23c TL |
104 | Describe your changes |
105 | --------------------- | |
7c673cae | 106 | |
9f95a23c TL |
107 | Each commit has an associated commit message that is stored in git. The first |
108 | line of the commit message is the `commit title`_. The second line should be | |
109 | left blank. The lines that follow constitute the `commit message`_. | |
7c673cae | 110 | |
9f95a23c | 111 | A commit and its message should be focused around a particular change. |
7c673cae | 112 | |
9f95a23c TL |
113 | Commit title |
114 | ^^^^^^^^^^^^ | |
7c673cae | 115 | |
9f95a23c TL |
116 | The text up to the first empty line in a commit message is the commit |
117 | title. It should be a single short line of at most 72 characters, | |
118 | summarizing the change, and prefixed with the | |
119 | subsystem or module you are changing. Also, it is conventional to use the | |
120 | imperative mood in the commit title. Positive examples include:: | |
7c673cae | 121 | |
9f95a23c TL |
122 | mds: add perf counter for finisher of MDSRank |
123 | osd: make the ClassHandler::mutex private | |
7c673cae | 124 | |
f67539c2 TL |
125 | If the change only touches the files under ``doc`` directory, the title |
126 | should start with "doc". For instance, a commit fixing a typo in | |
127 | ``doc/mgr/dashboard.rst`` could have a title like:: | |
128 | ||
129 | doc/mgr/dashboard: fix a typo | |
130 | ||
9f95a23c TL |
131 | More positive examples can be obtained from the git history of the ``master`` |
132 | branch:: | |
7c673cae | 133 | |
9f95a23c | 134 | git log |
7c673cae | 135 | |
9f95a23c | 136 | Some negative examples (how *not* to title a commit message):: |
7c673cae | 137 | |
9f95a23c TL |
138 | update driver X |
139 | bug fix for driver X | |
140 | fix issue 99999 | |
7c673cae | 141 | |
f67539c2 | 142 | Further to the last negative example ("fix issue 99999"), see `Fixes line(s)`_. |
7c673cae | 143 | |
9f95a23c TL |
144 | Commit message |
145 | ^^^^^^^^^^^^^^ | |
7c673cae | 146 | |
9f95a23c TL |
147 | (This section is about the body of the commit message. Please also see |
148 | the preceding section, `Commit title`_, for advice on titling commit messages.) | |
7c673cae | 149 | |
9f95a23c TL |
150 | In the body of your commit message, be as specific as possible. If the commit |
151 | message title was too short to fully state what the commit is doing, use the | |
152 | body to explain not just the "what", but also the "why". | |
7c673cae | 153 | |
9f95a23c TL |
154 | For positive examples, peruse ``git log`` in the ``master`` branch. A negative |
155 | example would be a commit message that merely states the obvious. For example: | |
156 | "this patch includes updates for subsystem X. Please apply." | |
7c673cae | 157 | |
9f95a23c TL |
158 | Fixes line(s) |
159 | ^^^^^^^^^^^^^ | |
7c673cae | 160 | |
9f95a23c TL |
161 | If the commit fixes one or more issues tracked by http://tracker.ceph.com, |
162 | add a ``Fixes:`` line (or lines) to the commit message, to connect this change | |
163 | to addressed issue(s) - for example:: | |
7c673cae | 164 | |
9f95a23c | 165 | Fixes: http://tracker.ceph.com/issues/12345 |
7c673cae | 166 | |
9f95a23c TL |
167 | This line should be added just before the ``Signed-off-by:`` line (see `Sign |
168 | your work`_). | |
7c673cae | 169 | |
9f95a23c TL |
170 | It helps reviewers to get more context of this bug and facilitates updating of |
171 | the bug tracker. Also, anyone perusing the git history will see this line and be | |
172 | able to refer to the bug tracker easily. | |
7c673cae | 173 | |
9f95a23c | 174 | Here is an example showing a properly-formed commit message:: |
7c673cae | 175 | |
9f95a23c | 176 | doc: add "--foo" option to bar |
7c673cae | 177 | |
9f95a23c TL |
178 | This commit updates the man page for bar with the newly added "--foo" |
179 | option. | |
7c673cae | 180 | |
9f95a23c TL |
181 | Fixes: http://tracker.ceph.com/issues/12345 |
182 | Signed-off-by: Random J Developer <random@developer.example.org> | |
7c673cae | 183 | |
9f95a23c TL |
184 | If a commit fixes a regression introduced by a different commit, please also |
185 | (in addition to the above) add a line referencing the SHA1 of the commit that | |
186 | introduced the regression. For example:: | |
7c673cae | 187 | |
9f95a23c | 188 | Fixes: 9dbe7a003989f8bb45fe14aaa587e9d60a392727 |
7c673cae | 189 | |
7c673cae | 190 | |
9f95a23c TL |
191 | PR best practices |
192 | ----------------- | |
7c673cae | 193 | |
9f95a23c TL |
194 | PRs should be opened on branches contained in your fork of |
195 | https://github.com/ceph/ceph.git - do not push branches directly to | |
196 | ``ceph/ceph.git``. | |
7c673cae | 197 | |
9f95a23c TL |
198 | PRs should target "master". If you need to add a patch to a stable branch, such |
199 | as "nautilus", see the file ``SubmittingPatches-backports.rst``. | |
7c673cae | 200 | |
9f95a23c TL |
201 | In addition to a base, or "target" branch, PRs have several other components: |
202 | the `PR title`_, the `PR description`_, labels, comments, etc. Of these, the PR | |
203 | title and description are relevant for new contributors. | |
7c673cae | 204 | |
9f95a23c TL |
205 | PR title |
206 | ^^^^^^^^ | |
7c673cae | 207 | |
9f95a23c TL |
208 | If your PR has only one commit, the PR title can be the same as the commit title |
209 | (and GitHub will suggest this). If the PR has multiple commits, do not accept | |
f67539c2 | 210 | the title GitHub suggests. Either use the title of the most relevant commit, or |
9f95a23c TL |
211 | write your own title. In the latter case, use the same "subsystem: short |
212 | description" convention described in `Commit title`_ for the PR title, with | |
213 | the following difference: the PR title describes the entire set of changes, | |
214 | while the `Commit title`_ describes only the changes in a particular commit. | |
7c673cae | 215 | |
f67539c2 TL |
216 | If GitHub suggests a PR title based on a very long commit message it will split |
217 | the result with an elipsis (...) and fold the remainder into the PR description. | |
218 | In such a case, please edit the title to be more concise and the description to | |
219 | remove the elipsis. | |
220 | ||
9f95a23c TL |
221 | Keep in mind that the PR titles feed directly into the script that generates |
222 | release notes and it is tedious to clean up non-conformant PR titles at release | |
223 | time. This document places no limit on the length of PR titles, but be aware | |
224 | that they are subject to editing as part of the release process. | |
7c673cae | 225 | |
9f95a23c TL |
226 | PR description |
227 | ^^^^^^^^^^^^^^ | |
7c673cae | 228 | |
9f95a23c | 229 | In addition to a title, the PR also has a description field, or "body". |
7c673cae | 230 | |
9f95a23c TL |
231 | The PR description is a place for summarizing the PR as a whole. It need not |
232 | duplicate information that is already in the commit messages. It can contain | |
233 | notices to maintainers, links to tracker issues and other related information, | |
234 | to-do lists, etc. The PR title and description should give readers a high-level | |
235 | notion of what the PR is about, quickly enabling them to decide whether they | |
236 | should take a closer look. | |
7c673cae | 237 | |
7c673cae | 238 | |
9f95a23c TL |
239 | Flag your changes for backport |
240 | ------------------------------ | |
7c673cae | 241 | |
9f95a23c TL |
242 | If you believe your changes should be backported to stable branches after the PR |
243 | is merged, open a tracker issue at https://tracker.ceph.com explaining: | |
7c673cae | 244 | |
9f95a23c TL |
245 | 1. what bug is fixed |
246 | 2. why does the bug need to be fixed in <release> | |
7c673cae | 247 | |
9f95a23c | 248 | and fill out the Backport field in the tracker issue. For example:: |
7c673cae | 249 | |
9f95a23c | 250 | Backport: mimic, nautilus |
7c673cae | 251 | |
9f95a23c TL |
252 | For information on how backports are done in the Ceph project, refer to the |
253 | document ``SubmittingPatches-backports.rst``. | |
7c673cae | 254 | |
7c673cae | 255 | |
9f95a23c TL |
256 | Test your changes |
257 | ----------------- | |
7c673cae | 258 | |
9f95a23c TL |
259 | Before opening your PR, it's a good idea to run tests on your patchset. Doing |
260 | that is simple, though the process can take a long time to complete, especially | |
261 | on older machines with less memory and spinning disks. | |
7c673cae | 262 | |
9f95a23c TL |
263 | The most simple test is to verify that your patchset builds, at least in your |
264 | own development environment. The commands for this are:: | |
7c673cae | 265 | |
9f95a23c TL |
266 | ./install-deps.sh |
267 | ./do_cmake.sh | |
268 | make | |
7c673cae | 269 | |
9f95a23c TL |
270 | Ceph comes with a battery of tests that can be run on a single machine. These |
271 | are collectively referred to as "make check", and can be run by executing the | |
272 | following command:: | |
7c673cae | 273 | |
9f95a23c | 274 | ./run-make-check.sh |
7c673cae | 275 | |
9f95a23c TL |
276 | If your patchset does not build, or if one or more of the "make check" tests |
277 | fails, but the error shown is not obviously related to your patchset, don't let | |
278 | that dissuade you from opening a PR. The Ceph project has a Jenkins instance | |
279 | which will build your PR branch and run "make check" on it in a controlled | |
280 | environment. | |
7c673cae | 281 | |
9f95a23c TL |
282 | Once your patchset builds and passes "make check", you can run even more tests |
283 | on it by issuing the following commands:: | |
7c673cae | 284 | |
9f95a23c TL |
285 | cd build |
286 | ../qa/run-standalone.sh | |
7c673cae | 287 | |
9f95a23c TL |
288 | Like "make check", the standalone tests take a long time to run. They also |
289 | produce voluminous output. If one or more of the standalone tests fails, it's | |
290 | likely the relevant part of the output will have scrolled off your screen or | |
291 | gotten swapped out of your buffer. Therefore, it makes sense to capture the | |
292 | output in a file for later analysis. | |
7c673cae | 293 | |
7c673cae | 294 | |
9f95a23c TL |
295 | Document your changes |
296 | --------------------- | |
c07f9fc5 | 297 | |
9f95a23c TL |
298 | If you have added or modified any user-facing functionality, such as CLI |
299 | commands or their output, then the pull request must include appropriate updates | |
300 | to documentation. | |
c07f9fc5 FG |
301 | |
302 | It is the submitter's responsibility to make the changes, and the reviewer's | |
303 | responsibility to make sure they are not merging changes that do not | |
304 | have the needed updates to documentation. | |
305 | ||
9f95a23c TL |
306 | Where there are areas that have absent documentation, or there is no clear place |
307 | to note the change that is being made, the reviewer should contact the component | |
308 | lead, who should arrange for the missing section to be created with sufficient | |
309 | detail for the PR submitter to document their changes. | |
7c673cae | 310 | |
11fdf7f2 TL |
311 | When writing and/or editing documentation, follow the Google Developer |
312 | Documentation Style Guide: https://developers.google.com/style/ |