]>
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 | ||
7c673cae FG |
69 | using your real name (sorry, no pseudonyms or anonymous contributions.) |
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 | |
9f95a23c TL |
125 | More positive examples can be obtained from the git history of the ``master`` |
126 | branch:: | |
7c673cae | 127 | |
9f95a23c | 128 | git log |
7c673cae | 129 | |
9f95a23c | 130 | Some negative examples (how *not* to title a commit message):: |
7c673cae | 131 | |
9f95a23c TL |
132 | update driver X |
133 | bug fix for driver X | |
134 | fix issue 99999 | |
7c673cae | 135 | |
9f95a23c | 136 | Further to the last negative example ("fix issue 99999"), see `Fixes line`_. |
7c673cae | 137 | |
9f95a23c TL |
138 | Commit message |
139 | ^^^^^^^^^^^^^^ | |
7c673cae | 140 | |
9f95a23c TL |
141 | (This section is about the body of the commit message. Please also see |
142 | the preceding section, `Commit title`_, for advice on titling commit messages.) | |
7c673cae | 143 | |
9f95a23c TL |
144 | In the body of your commit message, be as specific as possible. If the commit |
145 | message title was too short to fully state what the commit is doing, use the | |
146 | body to explain not just the "what", but also the "why". | |
7c673cae | 147 | |
9f95a23c TL |
148 | For positive examples, peruse ``git log`` in the ``master`` branch. A negative |
149 | example would be a commit message that merely states the obvious. For example: | |
150 | "this patch includes updates for subsystem X. Please apply." | |
7c673cae | 151 | |
9f95a23c | 152 | .. _`fixes line`: |
7c673cae | 153 | |
9f95a23c TL |
154 | Fixes line(s) |
155 | ^^^^^^^^^^^^^ | |
7c673cae | 156 | |
9f95a23c TL |
157 | If the commit fixes one or more issues tracked by http://tracker.ceph.com, |
158 | add a ``Fixes:`` line (or lines) to the commit message, to connect this change | |
159 | to addressed issue(s) - for example:: | |
7c673cae | 160 | |
9f95a23c | 161 | Fixes: http://tracker.ceph.com/issues/12345 |
7c673cae | 162 | |
9f95a23c TL |
163 | This line should be added just before the ``Signed-off-by:`` line (see `Sign |
164 | your work`_). | |
7c673cae | 165 | |
9f95a23c TL |
166 | It helps reviewers to get more context of this bug and facilitates updating of |
167 | the bug tracker. Also, anyone perusing the git history will see this line and be | |
168 | able to refer to the bug tracker easily. | |
7c673cae | 169 | |
9f95a23c | 170 | Here is an example showing a properly-formed commit message:: |
7c673cae | 171 | |
9f95a23c | 172 | doc: add "--foo" option to bar |
7c673cae | 173 | |
9f95a23c TL |
174 | This commit updates the man page for bar with the newly added "--foo" |
175 | option. | |
7c673cae | 176 | |
9f95a23c TL |
177 | Fixes: http://tracker.ceph.com/issues/12345 |
178 | Signed-off-by: Random J Developer <random@developer.example.org> | |
7c673cae | 179 | |
9f95a23c TL |
180 | If a commit fixes a regression introduced by a different commit, please also |
181 | (in addition to the above) add a line referencing the SHA1 of the commit that | |
182 | introduced the regression. For example:: | |
7c673cae | 183 | |
9f95a23c | 184 | Fixes: 9dbe7a003989f8bb45fe14aaa587e9d60a392727 |
7c673cae | 185 | |
7c673cae | 186 | |
9f95a23c TL |
187 | PR best practices |
188 | ----------------- | |
7c673cae | 189 | |
9f95a23c TL |
190 | PRs should be opened on branches contained in your fork of |
191 | https://github.com/ceph/ceph.git - do not push branches directly to | |
192 | ``ceph/ceph.git``. | |
7c673cae | 193 | |
9f95a23c TL |
194 | PRs should target "master". If you need to add a patch to a stable branch, such |
195 | as "nautilus", see the file ``SubmittingPatches-backports.rst``. | |
7c673cae | 196 | |
9f95a23c TL |
197 | In addition to a base, or "target" branch, PRs have several other components: |
198 | the `PR title`_, the `PR description`_, labels, comments, etc. Of these, the PR | |
199 | title and description are relevant for new contributors. | |
7c673cae | 200 | |
9f95a23c TL |
201 | PR title |
202 | ^^^^^^^^ | |
7c673cae | 203 | |
9f95a23c TL |
204 | If your PR has only one commit, the PR title can be the same as the commit title |
205 | (and GitHub will suggest this). If the PR has multiple commits, do not accept | |
206 | the title GitHub suggest. Either use the title of the most relevant commit, or | |
207 | write your own title. In the latter case, use the same "subsystem: short | |
208 | description" convention described in `Commit title`_ for the PR title, with | |
209 | the following difference: the PR title describes the entire set of changes, | |
210 | while the `Commit title`_ describes only the changes in a particular commit. | |
7c673cae | 211 | |
9f95a23c TL |
212 | Keep in mind that the PR titles feed directly into the script that generates |
213 | release notes and it is tedious to clean up non-conformant PR titles at release | |
214 | time. This document places no limit on the length of PR titles, but be aware | |
215 | that they are subject to editing as part of the release process. | |
7c673cae | 216 | |
9f95a23c TL |
217 | PR description |
218 | ^^^^^^^^^^^^^^ | |
7c673cae | 219 | |
9f95a23c | 220 | In addition to a title, the PR also has a description field, or "body". |
7c673cae | 221 | |
9f95a23c TL |
222 | The PR description is a place for summarizing the PR as a whole. It need not |
223 | duplicate information that is already in the commit messages. It can contain | |
224 | notices to maintainers, links to tracker issues and other related information, | |
225 | to-do lists, etc. The PR title and description should give readers a high-level | |
226 | notion of what the PR is about, quickly enabling them to decide whether they | |
227 | should take a closer look. | |
7c673cae | 228 | |
7c673cae | 229 | |
9f95a23c TL |
230 | Flag your changes for backport |
231 | ------------------------------ | |
7c673cae | 232 | |
9f95a23c TL |
233 | If you believe your changes should be backported to stable branches after the PR |
234 | is merged, open a tracker issue at https://tracker.ceph.com explaining: | |
7c673cae | 235 | |
9f95a23c TL |
236 | 1. what bug is fixed |
237 | 2. why does the bug need to be fixed in <release> | |
7c673cae | 238 | |
9f95a23c | 239 | and fill out the Backport field in the tracker issue. For example:: |
7c673cae | 240 | |
9f95a23c | 241 | Backport: mimic, nautilus |
7c673cae | 242 | |
9f95a23c TL |
243 | For information on how backports are done in the Ceph project, refer to the |
244 | document ``SubmittingPatches-backports.rst``. | |
7c673cae | 245 | |
7c673cae | 246 | |
9f95a23c TL |
247 | Test your changes |
248 | ----------------- | |
7c673cae | 249 | |
9f95a23c TL |
250 | Before opening your PR, it's a good idea to run tests on your patchset. Doing |
251 | that is simple, though the process can take a long time to complete, especially | |
252 | on older machines with less memory and spinning disks. | |
7c673cae | 253 | |
9f95a23c TL |
254 | The most simple test is to verify that your patchset builds, at least in your |
255 | own development environment. The commands for this are:: | |
7c673cae | 256 | |
9f95a23c TL |
257 | ./install-deps.sh |
258 | ./do_cmake.sh | |
259 | make | |
7c673cae | 260 | |
9f95a23c TL |
261 | Ceph comes with a battery of tests that can be run on a single machine. These |
262 | are collectively referred to as "make check", and can be run by executing the | |
263 | following command:: | |
7c673cae | 264 | |
9f95a23c | 265 | ./run-make-check.sh |
7c673cae | 266 | |
9f95a23c TL |
267 | If your patchset does not build, or if one or more of the "make check" tests |
268 | fails, but the error shown is not obviously related to your patchset, don't let | |
269 | that dissuade you from opening a PR. The Ceph project has a Jenkins instance | |
270 | which will build your PR branch and run "make check" on it in a controlled | |
271 | environment. | |
7c673cae | 272 | |
9f95a23c TL |
273 | Once your patchset builds and passes "make check", you can run even more tests |
274 | on it by issuing the following commands:: | |
7c673cae | 275 | |
9f95a23c TL |
276 | cd build |
277 | ../qa/run-standalone.sh | |
7c673cae | 278 | |
9f95a23c TL |
279 | Like "make check", the standalone tests take a long time to run. They also |
280 | produce voluminous output. If one or more of the standalone tests fails, it's | |
281 | likely the relevant part of the output will have scrolled off your screen or | |
282 | gotten swapped out of your buffer. Therefore, it makes sense to capture the | |
283 | output in a file for later analysis. | |
7c673cae | 284 | |
7c673cae | 285 | |
9f95a23c TL |
286 | Document your changes |
287 | --------------------- | |
c07f9fc5 | 288 | |
9f95a23c TL |
289 | If you have added or modified any user-facing functionality, such as CLI |
290 | commands or their output, then the pull request must include appropriate updates | |
291 | to documentation. | |
c07f9fc5 FG |
292 | |
293 | It is the submitter's responsibility to make the changes, and the reviewer's | |
294 | responsibility to make sure they are not merging changes that do not | |
295 | have the needed updates to documentation. | |
296 | ||
9f95a23c TL |
297 | Where there are areas that have absent documentation, or there is no clear place |
298 | to note the change that is being made, the reviewer should contact the component | |
299 | lead, who should arrange for the missing section to be created with sufficient | |
300 | detail for the PR submitter to document their changes. | |
7c673cae | 301 | |
11fdf7f2 TL |
302 | When writing and/or editing documentation, follow the Google Developer |
303 | Documentation Style Guide: https://developers.google.com/style/ |