You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
-[9. It's OK to Push Back](#9-its-ok-to-push-back)
32
+
-[10. Common Sense and Courtesy](#10-common-sense-and-courtesy)
33
+
-[11. Trivial Edits](#11-trivial-edits)
33
34
34
35
# Before You Submit a Pull Request
35
36
@@ -280,7 +281,7 @@ Nothing is more frustrating than starting a review, only to find that the tests
280
281
281
282
If you don't know how to test Feature-X, please ask! We'll be happy to help you design things for easy testing or to suggest appropriate test cases.
282
283
283
-
## 6. Squashing and Commit Titles
284
+
## 6. Squashing
284
285
285
286
Your reviewer has finally sent you feedback on Feature-X.
286
287
@@ -302,34 +303,260 @@ For more information, see [squash commits](./github-workflow.md#squash-commits).
302
303
303
304
Don't squash when there are independent changes layered to achieve a single goal. For instance, writing a code munger could be one commit, applying it could be another, and adding a precommit check could be a third. One could argue they should be separate pull requests, but there's really no way to test/review the munger without seeing it applied, and there needs to be a precommit check to ensure the munged output doesn't immediately get out of date.
304
305
305
-
A commit, as much as possible, should be a single logical change.
306
+
## 7. Commit Message Guidelines
306
307
307
-
## 7. KISS, YAGNI, MVP, etc.
308
+
PR comments are not represented in the commit history. Commits and their commit
309
+
messages are the _"permanent record"_ of the changes being done in your PR and
310
+
their commit messages should accurately describe both _what_ and _why_ it is
311
+
being done.
308
312
309
-
Sometimes we need to remind each other of core tenets of software design - Keep It Simple, You Aren't Gonna Need It, Minimum Viable Product, and so on. Adding a feature "because we might need it later" is antithetical to software that ships. Add the things you need NOW and (ideally) leave room for things you might need later - but don't implement them now.
313
+
Commit messages are comprised of two parts; the subject and the body.
310
314
311
-
## 8. It's OK to Push Back
315
+
The subject is the first line of the commit message and is often the only part
316
+
that is needed for small or trivial changes. Those may be done as "one liners"
317
+
with the `git commit -m` or the `--message` flag, but only if the what and
318
+
especially why can be fully described in that few words.
312
319
313
-
Sometimes reviewers make mistakes. It's OK to push back on changes your reviewer requested. If you have a good reason for doing something a certain way, you are absolutely allowed to debate the merits of a requested change. Both the reviewer and reviewee should strive to discuss these issues in a polite and respectful manner.
320
+
The commit message body is the portion of text below the subject when you run
321
+
`git commit` without the `-m` flag which will open the commit message for editing
322
+
in your [preferred editor]. Typing a few further sentences of clarification is
323
+
a useful investment in time both for your reviews and overall later project
324
+
maintenance.
325
+
326
+
```
327
+
This is the commit message subject
328
+
329
+
Any text here is the commit message body
330
+
Some text
331
+
Some more text
332
+
...
333
+
334
+
# Please enter the commit message for your changes. Lines starting
335
+
# with '#' will be ignored, and an empty message aborts the commit.
336
+
#
337
+
# On branch example
338
+
# Changes to be committed:
339
+
# ...
340
+
#
341
+
```
342
+
343
+
Use these guidelines below to help craft a well formatted commit message. These
344
+
can be largely attributed to the previous work of [Chris Beams], [Tim Pope],
345
+
[Scott Chacon] and [Ben Straub].
346
+
347
+
-[Try to keep the subject line to 50 characters or less; do not exceed 72 characters](#try-to-keep-the-subject-line-to-50-characters-or-less-do-not-exceed-72-characters)
348
+
-[The first word in the commit message subject should be capitalized unless it starts with a lowercase symbol or other identifier](#the-first-word-in-the-commit-message-subject-should-be-capitalized-unless-it-starts-with-a-lowercase-symbol-or-other-identifier)
349
+
-[Do not end the commit message subject with a period](#do-not-end-the-commit-message-subject-with-a-period)
350
+
-[Use imperative mood in your commit message subject](#use-imperative-mood-in-your-commit-message-subject)
351
+
-[Add a single blank line before the commit message body](#add-a-single-blank-line-before-the-commit-message-body)
352
+
-[Wrap the commit message body at 72 characters](#wrap-the-commit-message-body-at-72-characters)
353
+
-[Do not use GitHub keywords or (@)mentions within your commit message](#do-not-use-github-keywords-or-mentions-within-your-commit-message)
354
+
-[Use the commit message body to explain the _what_ and _why_ of the commit](#use-the-commit-message-body-to-explain-the-what-and-why-of-the-commit)
355
+
356
+
<!-- omit in toc -->
357
+
### Try to keep the subject line to 50 characters or less; do not exceed 72 characters
358
+
359
+
The 50 character limit for the commit message subject line acts as a focus to
360
+
keep the message summary as concise as possible. It should be just enough to
361
+
describe what is being done.
362
+
363
+
The hard limit of 72 characters is to align with the max body size. When viewing
364
+
the history of a repository with `git log`, git will pad the body text with
365
+
additional blank spaces. Wrapping the width at 72 characters ensures the body
366
+
text will be centered and easily viewable on an 80-column terminal.
367
+
368
+
<!-- omit in toc -->
369
+
#### Providing additional context
370
+
371
+
You can provide additional context with fewer characters by prefixing your
372
+
commit message with the [kind] or [area] that your PR is impacting. These are
373
+
commonly used labels that other members of the Kubernetes community will
374
+
understand.
375
+
376
+
**Examples:**
377
+
-`cleanup: remove unused portion of script foo`
378
+
-`deprecation: add notice for bar feature removal in future release`
379
+
-`etcd: update default server to 3.4.7`
380
+
-`kube-proxy: add a test case for HostnameOverride`
381
+
382
+
These can serve as a good subject before expanding further on the what and why
383
+
within the commit message body.
384
+
385
+
386
+
<!-- omit in toc -->
387
+
### The first word in the commit message subject should be capitalized unless it starts with a lowercase symbol or other identifier
388
+
389
+
The commit message subject is like an abbreviated sentence. The first word should
390
+
be capitalized unless the message begins with symbol, acronym or other identifier
391
+
such as [kind] or [area] that would regularly be lowercase.
392
+
393
+
394
+
<!-- omit in toc -->
395
+
### Do not end the commit message subject with a period
396
+
397
+
This is primary intended to serve as a space saving measure, but also aids in
398
+
driving the subject line to be as short and concise as possible.
399
+
400
+
401
+
<!-- omit in toc -->
402
+
### Use imperative mood in your commit message subject
403
+
404
+
Imperative mood can be be thought of as a _"giving a command"_; it is a
405
+
**present-tense** statement that explicitly describes what is being done.
406
+
407
+
**Good Examples:**
408
+
- Fix x error in y
409
+
- Add foo to bar
410
+
- Revert commit "baz"
411
+
- Update pull request guidelines
412
+
413
+
**Bad Examples**
414
+
- Fixed x error in y
415
+
- Added foo to bar
416
+
- Reverting bad commit "baz"
417
+
- Updating the pull request guidelines
418
+
- Fixing more things
419
+
420
+
A general guideline from [Chris Beams] on forming an imperative commit subject
421
+
is it should complete this sentence:
422
+
```
423
+
If applied, this commit will <your subject line here>
424
+
```
425
+
426
+
**Examples:**
427
+
-_If applied, this commit will_**Fix x error in y**
428
+
-_If applied, this commit will_**Add foo to bar**
429
+
-_If applied, this commit will_**Revert commit "baz"**
430
+
-_If applied, this commit will_**Update the pull request guidelines**
314
431
315
-
You might be overruled, but you might also prevail. We're pretty reasonable people. Mostly.
316
432
317
-
Another phenomenon of open-source projects (where anyone can comment on any issue) is the dog-pile - your pull request gets so many comments from so many people it becomes hard to follow. In this situation, you can ask the primary reviewer (assignee) whether they want you to fork a new pull request to clear out all the comments. You don't HAVE to fix every issue raised by every person who feels like commenting, but you should answer reasonable comments with an explanation.
433
+
<!-- omit in toc -->
434
+
### Add a single blank line before the commit message body
318
435
319
-
## 9. Common Sense and Courtesy
436
+
Git uses the blank line to determine which portion of the commit message is the
437
+
subject and body. Text preceding the blank line is the subject, and text
438
+
following is considered the body.
320
439
321
-
No document can take the place of common sense and good taste. Use your best judgment, while you put
322
-
a bit of thought into how your work can be made easier to review. If you do these things your pull requests will get merged with less friction.
440
+
<!-- omit in toc -->
441
+
### Wrap the commit message body at 72 characters
323
442
324
-
## 10. Trivial Edits
443
+
The default column width for git is 80 characters. Git will pad the text of the
444
+
message body with an additional 4 spaces when viewing the git log. This would
445
+
leave you with 76 available spaces for text, however the text would be "lop-sided".
446
+
To center the text for better viewing, the other side is artificially padded
447
+
with the same amount of spaces, resulting in 72 usable characters per line. Think
448
+
of them as the margins in a word doc.
449
+
450
+
451
+
<!-- omit in toc -->
452
+
### Do not use GitHub keywords or (@)mentions within your commit message
453
+
454
+
<!-- omit in toc -->
455
+
#### GitHub Keywords
456
+
457
+
Using [GitHub keywords] followed by a `#<issue number>` reference within your
458
+
commit message will automatically apply the `do-not-merge/invalid-commit-message`
459
+
label to your PR preventing it from being merged.
460
+
461
+
[GitHub keywords] in a PR to close issues is considered a convenience item, but
462
+
can have unexpected side-effects; often closing something they shouldn't.
463
+
464
+
**Blocked Keywords:**
465
+
- close
466
+
- closes
467
+
- closed
468
+
- fix
469
+
- fixes
470
+
- fixed
471
+
- resolve
472
+
- resolves
473
+
- resolved
474
+
475
+
476
+
<!-- omit in toc -->
477
+
#### (@)Mentions
478
+
479
+
(@)mentions within the commit message will send a notification to that user, and
480
+
will continually do so each time the PR is updated.
481
+
482
+
483
+
<!-- omit in toc -->
484
+
### Use the commit message body to explain the _what_ and _why_ of the commit
485
+
486
+
Commits and their commit messages are the _"permanent record"_ of the changes
487
+
being done in your PR. Describing why something has changed and what effects it
488
+
may have. You are providing context to both your reviewer and the next person
489
+
that has to touch your code.
490
+
491
+
If something is resolving a bug, or is in response to a specific issue, you can
492
+
link to it as a reference with the message body itself. These sorts of
493
+
breadcrumbs become essential when tracking down future bugs or regressions and
494
+
further help explain the _"why"_ the commit was made.
495
+
496
+
497
+
**Additional Resources:**
498
+
-[How to Write a Git Commit Message - Chris Beams](https://chris.beams.io/posts/git-commit/)
499
+
-[Distributed Git - Contributing to a Project (Commit Guidelines)](https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project)
500
+
-[What’s with the 50/72 rule? - Preslav Rachev](https://preslav.me/2015/02/21/what-s-with-the-50-72-rule/)
501
+
-[A Note About Git Commit Messages - Tim Pope](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html)
502
+
503
+
504
+
## 8. KISS, YAGNI, MVP, etc.
505
+
506
+
Sometimes we need to remind each other of core tenets of software design - Keep
507
+
It Simple, You Aren't Gonna Need It, Minimum Viable Product, and so on. Adding a
508
+
feature "because we might need it later" is antithetical to software that ships.
509
+
Add the things you need NOW and (ideally) leave room for things you might need
510
+
later - but don't implement them now.
511
+
512
+
## 9. It's OK to Push Back
513
+
514
+
Sometimes reviewers make mistakes. It's OK to push back on changes your reviewer
515
+
requested. If you have a good reason for doing something a certain way, you are
516
+
absolutely allowed to debate the merits of a requested change. Both the reviewer
517
+
and reviewee should strive to discuss these issues in a polite and respectful manner.
518
+
519
+
You might be overruled, but you might also prevail. We're pretty reasonable people.
520
+
521
+
Another phenomenon of open-source projects (where anyone can comment on any issue)
522
+
is the dog-pile - your pull request gets so many comments from so many people it
523
+
becomes hard to follow. In this situation, you can ask the primary reviewer
524
+
(assignee) whether they want you to fork a new pull request to clear out all the
525
+
comments. You don't HAVE to fix every issue raised by every person who feels
526
+
like commenting, but you should answer reasonable comments with an explanation.
527
+
528
+
## 10. Common Sense and Courtesy
529
+
530
+
No document can take the place of common sense and good taste. Use your best
531
+
judgment, while you put
532
+
a bit of thought into how your work can be made easier to review. If you do
533
+
these things your pull requests will get merged with less friction.
534
+
535
+
## 11. Trivial Edits
325
536
326
537
Each incoming Pull Request needs to be reviewed, checked, and then merged.
327
538
328
-
While automation helps with this, each contribution also has an engineering cost. Therefore it is appreciated if you do NOT make trivial edits and fixes, but instead focus on giving the entire file a review.
539
+
While automation helps with this, each contribution also has an engineering cost.
540
+
Therefore it is appreciated if you do NOT make trivial edits and fixes, but
541
+
instead focus on giving the entire file a review.
329
542
330
-
If you find one grammatical or spelling error, it is likely there are more in that file, you can really make your Pull Request count by checking the formatting, checking for broken links, and fixing errors and then submitting all the fixes at once to that file.
543
+
If you find one grammatical or spelling error, it is likely there are more in
544
+
that file, you can really make your Pull Request count by checking the formatting,
545
+
checking for broken links, and fixing errors and then submitting all the fixes
546
+
at once to that file.
331
547
332
548
**Some questions to consider:**
333
549
334
550
* Can the file be improved further?
335
551
* Does the trivial edit greatly improve the quality of the content?
0 commit comments