kubernetes
/
community
mirror of https://github.com/kubernetes/community.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Deduplify content: Remove reviewee content from the review-guidelines (#5031) 

* Deduplify content: Remove reviewee content from the review-guidelines,
reorganize pull request page.

Signed-off-by: Jorge O. Castro <jorgec@vmware.com>

* Reformat ToC headers, move KISS section higher

Signed-off-by: Jorge O. Castro <jorgec@vmware.com>
This commit is contained in:
Jorge O. Castro 2020-08-17 20:09:30 -04:00 committed by GitHub
parent 042df2dede
commit d27cb26831
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 247 additions and 323 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
contributors/guide/contributing.md
View File

@ -41,13 +41,17 @@ For general questions and troubleshooting, use the [standard lines of communicat
To check out code to work on, please refer to [the GitHub Workflow Guide](./github-workflow.md).
## Open a Pull Request
The full workflow for a pull request is documented here:
- [Kubernetes-specific github workflow](pull-requests.md#the-testing-and-merge-workflow).
That document is comprehensive and detailed, for purposes of a typical pull request we will cover the initial and simple use case here:
## Opening a Pull Request
Pull requests are often called a "PR".
Kubernetes generally follows the standard [github pull request](https://help.github.com/articles/about-pull-requests/) process, but there is a layer of additional kubernetes specific (and sometimes SIG specific) differences:
- [Kubernetes-specific github workflow](pull-requests.md#the-testing-and-merge-workflow).
The first difference you'll see is that a bot will begin applying structured labels to your PR.
The bot may also make some helpful suggestions for commands to run in your PR to facilitate review.
@ -127,5 +131,6 @@ If you're looking to run e2e tests on your own infrastructure, [kubetest](https:
## Issues Management or Triage
Have you ever noticed the total number of [open issues](https://issues.k8s.io)?
Helping to manage or triage these open issues can be a great contribution and a great opportunity to learn about the various areas of the project. Triaging is the word we use to describe the process of adding multiple types of descriptive labels to GitHub issues, in order to speed up routing issues to the right folks.
Helping to manage or triage these open issues can be a great contribution and a great opportunity to learn about the various areas of the project.
Triaging is the word we use to describe the process of adding multiple types of descriptive labels to GitHub issues, in order to speed up routing issues to the right folks.
Refer to the [Issue Triage Guidelines](/contributors/guide/issue-triage.md) for more information.

424
contributors/guide/pull-requests.md
View File

@ -8,13 +8,12 @@ description: |
infrequent submitters.
---
This doc explains the process and best practices for submitting a pull request to the [Kubernetes project](https://github.com/kubernetes/kubernetes) and its associated sub-repositories. It should serve as a reference for all contributors, and be useful especially to new and infrequent submitters.
This doc explains the process and best practices for submitting a pull request to the [Kubernetes project](https://github.com/kubernetes/kubernetes) and its associated sub-repositories.
It should serve as a reference for all contributors, and be useful especially to new and infrequent submitters.
- [Before You Submit a Pull Request](#before-you-submit-a-pull-request)
- [Run Local Verifications](#run-local-verifications)
- [The Pull Request Submit Process](#the-pull-request-submit-process)
- [The Testing and Merge Workflow](#the-testing-and-merge-workflow)
- [More About `Ok-To-Test`](#more-about-ok-to-test)
- [Marking Unfinished Pull Requests](#marking-unfinished-pull-requests)
- [Pull Requests and the Release Cycle](#pull-requests-and-the-release-cycle)
- [Comment Commands Reference](#comment-commands-reference)
@ -23,31 +22,35 @@ This doc explains the process and best practices for submitting a pull request t
- [Why was my pull request closed?](#why-was-my-pull-request-closed)
- [Why is my pull request not getting reviewed?](#why-is-my-pull-request-not-getting-reviewed)
- [Best Practices for Faster Reviews](#best-practices-for-faster-reviews)
- [0. Familiarize yourself with project conventions](#0-familiarize-yourself-with-project-conventions)
- [1. Is the feature wanted? File a Kubernetes Enhancement Proposal](#1-is-the-feature-wanted-file-a-kubernetes-enhancement-proposal)
- [2. Smaller Is Better: Small Commits, Small Pull Requests](#2-smaller-is-better-small-commits-small-pull-requests)
- [3. Open a Different Pull Request for Fixes and Generic Features](#3-open-a-different-pull-request-for-fixes-and-generic-features)
- [4. Comments Matter](#4-comments-matter)
- [5. Test](#5-test)
- [6. Squashing](#6-squashing)
- [7. Commit Message Guidelines](#7-commit-message-guidelines)
- [8. KISS, YAGNI, MVP, etc.](#8-kiss-yagni-mvp-etc)
- [9. It's OK to Push Back](#9-its-ok-to-push-back)
- [10. Common Sense and Courtesy](#10-common-sense-and-courtesy)
- [11. Trivial Edits](#11-trivial-edits)
- [Familiarize yourself with project conventions](#familiarize-yourself-with-project-conventions)
- [Is the feature wanted? File a Kubernetes Enhancement Proposal](#is-the-feature-wanted-file-a-kubernetes-enhancement-proposal)
- [Smaller Is Better: Small Commits, Small Pull Requests](#smaller-is-better-small-commits-small-pull-requests)
- [Open a Different Pull Request for Fixes and Generic Features](#open-a-different-pull-request-for-fixes-and-generic-features)
- [Comments Matter](#comments-matter)
- [Test](#test)
- [Squashing](#squashing)
- [Commit Message Guidelines](#commit-message-guidelines)
- [KISS, YAGNI, MVP, etc.](#kiss-yagni-mvp-etc)
- [It's OK to Push Back](#its-ok-to-push-back)
- [Common Sense and Courtesy](#common-sense-and-courtesy)
- [Trivial Edits](#trivial-edits)
- [The Testing and Merge Workflow](#the-testing-and-merge-workflow)
- [More About `Ok-To-Test`](#more-about-ok-to-test)
# Before You Submit a Pull Request
This guide is for contributors who already have a pull request to submit. If you're looking for information on setting up your developer environment and creating code to contribute to Kubernetes, see the [development guide](/contributors/devel/development.md).
This guide is for contributors who already have a pull request to submit.
If you're looking for information on setting up your developer environment and creating code to contribute to Kubernetes, see the [development guide](/contributors/devel/development.md).
First-time contributors should head to the [Contributor Guide](/contributors/guide/README.md) to get started.
**Make sure your pull request adheres to our best practices. These include following project conventions, making small pull requests, and commenting thoroughly. Please read the more detailed section on [Best Practices for Faster Reviews](#best-practices-for-faster-reviews) at the end of this doc.**
**Make sure your pull request adheres to our best practices.
These include following project conventions, making small pull requests, and commenting thoroughly. Please read the more detailed section on [Best Practices for Faster Reviews](#best-practices-for-faster-reviews) at the end of this doc.**
## Run Local Verifications
You can run these local verifications before you submit your pull request to predict the
pass or fail of continuous integration.
You can run these local verifications before you submit your pull request to predict the pass or fail of continuous integration.
* Run and pass `make verify` (can take 30-40 minutes)
* Run and pass `make test`
@ -63,111 +66,49 @@ Merging a pull request requires the following steps to be completed before the p
- Pass all e2e tests
- Get all necessary approvals from reviewers and code owners
## The Testing and Merge Workflow
The Kubernetes merge workflow uses labels, applied by [commands](https://prow.k8s.io/command-help) via comments. These will trigger actions on your pull request. Different Kubernetes repositories may require different labels on the path to approval. A generic explanation of how labels are used in pull requests can be found [here](/contributors/guide/owners.md#code-review-using-owners-files). The pull request bot will also automatically apply and/or suggest labels.
_Example:_ To apply a SIG label, you would type in a comment:
```
/sig apps
```
*NOTE: For pull requests that are in progress but not ready for review,
prefix the pull request title with `WIP` or `[WIP]` and track any remaining TODOs
in a checklist in the pull request description.*
Here's the process the pull request goes through on its way from submission to merging:
1. Make the pull request
1. `@k8s-ci-robot` assigns reviewers
1. If you're **not** a member of the Kubernetes organization, a Reviewer/Kubernetes Member checks that the pull request is safe to test. If so, they comment `/ok-to-test`. Pull requests by Kubernetes organization [members](/community-membership.md) do not need this step. Now the pull request is considered to be trusted, and the pre-submit tests will run:
1. Automatic tests run. See the current list of tests at this [link](https://prow.k8s.io/?repo=kubernetes%2Fkubernetes&type=presubmit)
1. If tests fail, resolve issues by pushing edits to your pull request branch
1. If the failure is a flake, anyone on trusted pull requests can comment `/retest` to rerun failed tests
1. Reviewer suggests edits
1. Push edits to your pull request branch
1. Repeat the prior two steps as needed until the reviewer(s) add `/lgtm` label. The `/lgtm` label, when applied by someone listed as a `reviewer` in the corresponding project `OWNERS` file, is a signal that the code has passed review from one or more trusted reviewers for that project
1. (Optional) Some reviewers prefer that you squash commits at this step
1. Follow the bot suggestions to assign an OWNER who will add the `/approve` label to the pull request. The `/approve` label, when applied by someone listed as an `approver` in the corresponding project `OWNERS`, is a signal that the code has passed final review and is ready to be automatically merged
The behavior of Prow is configurable across projects. You should be aware of the following configurable behaviors.
* If you are listed as an `/approver` in the `OWNERS` file, an implicit `/approve` can be applied to your pull request. This can result in a merge being triggered by a `/lgtm` label. This is the configured behavior in many projects, including `kubernetes/kubernetes`. You can remove the implicit `/approve` with `/approve cancel`
* `/lgtm` can be configured so that from someone listed as both a `reviewer` and an `approver` will cause both labels to be applied. For `kubernetes/kuebernetes` and many other projects this is _not_ the default behavior, and `/lgtm` is decoupled from `/approve`
Once the tests pass and the reviewer adds the `lgtm` and `approved` labels, the pull request enters the final merge pool. The merge pool is needed to make sure no incompatible changes have been introduced by other pull requests since the tests were last run on your pull request.
<!-- TODO: create parallel instructions for reviewers -->
[Tide](https://git.k8s.io/test-infra/prow/cmd/tide) will manage the merge pool
automatically. It uses GitHub queries to select PRs into “tide pools”,
runs as many in a batch as it can (“tide comes in”), and merges them (“tide goes out”).
1. The pull request enters the [merge pool](https://prow.k8s.io/tide)
if the merge criteria are met. The [PR dashboard](https://prow.k8s.io/pr) shows
the difference between your PR's state and the merge criteria so that you can
easily see all criteria that are not being met and address them.
1. If tests fail, resolve issues by pushing edits to your pull request branch
1. If the failure is a flake, anyone can comment `/retest` if the pull request is trusted
1. If tests pass, Tide automatically merges the pull request
That's the last step. Your pull request is now merged.
## More About `Ok-To-Test`
- The ok-to-test label is applied by org members to PRs from external contributors, it signals that the PR can be tested.
- For a Contributor, an `ok-to-test` label means the regular CI tests will be run for their PR.
- For the reviewer or the member, labelling the PR with `ok-to-test` it means a lot more:
- They need to take care if the PR is not a wastage of our `CI/CD` resources.
- Is the PR worth testing or does it need more changes before going through the `CI/CD` process?
- Is the PR getting used to run malicious code to misuse our resources ?
- An `ok-to-test` label may reduce the workload and smoothens the contributors experience as they can know if there is any failing test. If there is, you can fix the test and they don't have to wait for a long time to get a review from `maintainer/assignee`.
- There are various other factors on which labelling of `ok-to-test` depends :
- Size of PR :
- If the PR is of `size/S` or `size/M` which is just to fix a grammatical error or spelling mistake, the reviewer can trigger the `CI/CD` without having a second thought.
- If the PR is of `size/XXL` which aims at adding a new feature, a new API endpoint or any new substantial feature. There needs to other conventions & process to be followed regarding the change made. Hence it may have a slight to delay to get labelled with `ok-to-test`.
- Other org members who are not assigned to the following PR may also label `ok-to-test` , if the change is small.
- If the PR is labelled with `cncf-cla: no`, then it is better to wait before labelling `ok-to-test`.
- PRs with tag `do-not-merge/hold` or `needs-rebase` should make the appropriate changes before the PR can be labelled `ok-to-test`.
- PRs created by mistake without to meaningful change of code should not be labelled `ok-to-test` and closed.
## Marking Unfinished Pull Requests
If you want to solicit reviews before the implementation of your pull request is complete, you should hold your pull request to ensure that Tide does not pick it up and attempt to merge it. There are two methods to achieve this:
If you want to solicit reviews before the implementation of your pull request is complete, you should hold your pull request to ensure that Tide does not pick it up and attempt to merge it.
There are two methods to achieve this:
1. You may add the `/hold` or `/hold cancel` comment commands
2. You may add or remove a `WIP` or `[WIP]` prefix to your pull request title
The GitHub robots will add and remove the `do-not-merge/hold` label as you use the comment commands and the `do-not-merge/work-in-progress` label as you edit your title. While either label is present, your pull request will not be considered for merging.
The GitHub robots will add and remove the `do-not-merge/hold` label as you use the comment commands and the `do-not-merge/work-in-progress` label as you edit your title.
While either label is present, your pull request will not be considered for merging.
## Pull Requests and the Release Cycle
If a pull request has been reviewed but held or not approved, it might be due to the current phase in the [Release Cycle](/contributors/devel/sig-release/release.md). Occasionally, a SIG may freeze their own code base when working towards a specific feature or goal that could impact other development. During this time, your pull request could remain unmerged while their release work is completed.
If a pull request has been reviewed but held or not approved, it might be due to the current phase in the [Release Cycle](/contributors/devel/sig-release/release.md).
Occasionally, a SIG may freeze their own code base when working towards a specific feature or goal that could impact other development.
During this time, your pull request could remain unmerged while their release work is completed.
If you feel your pull request is in this state, contact the appropriate [SIG](https://git.k8s.io/community/sig-list.md) or [SIG-Release](https://git.k8s.io/sig-release) for clarification.
Check the [The Testing and Merge Workflow](#the-testing-and-merge-workflow) at the end of this document if you're interested in the details on how exactly the automation processes pull requests.
## Comment Commands Reference
[The commands doc](https://go.k8s.io/bot-commands) contains a reference for all comment commands.
## Automation
The Kubernetes developer community uses a variety of automation to manage pull requests. This automation is described in detail [in the automation doc](/contributors/devel/automation.md).
The Kubernetes developer community uses a variety of automation to manage pull requests.
This automation is described in detail [in the automation doc](/contributors/devel/automation.md).
## How the e2e Tests Work
The end-to-end tests will post the status results to the pull request. If an e2e test fails,
`@k8s-ci-robot` will comment on the pull request with the test history and the
comment-command to re-run that test. e.g.
The end-to-end tests will post the status results to the pull request.
If an e2e test fails, `@k8s-ci-robot` will comment on the pull request with the test history and the comment-command to re-run that test. e.g.
> The following tests failed, say /retest to rerun them all.
# Why was my pull request closed?
Pull requests older than 90 days will be closed. Exceptions can be made for pull requests that have active review comments, or that are awaiting other dependent pull requests. Closed pull requests are easy to recreate, and little work is lost by closing a pull request that subsequently needs to be reopened. We want to limit the total number of pull requests in flight to:
Pull requests older than 90 days will be closed.
Exceptions can be made for pull requests that have active review comments, or that are awaiting other dependent pull requests.
Closed pull requests are easy to recreate, and little work is lost by closing a pull request that subsequently needs to be reopened.
We want to limit the total number of pull requests in flight to:
* Maintain a clean project
* Remove old pull requests that would be difficult to rebase as the underlying code has changed over time
* Encourage code velocity
@ -178,12 +119,15 @@ A few factors affect how long your pull request might wait for review.
If it's the last few weeks of a milestone, we need to reduce churn and stabilize.
Or, it could be related to best practices. One common issue is that the pull request is too big to review. Let's say you've touched 39 files and have 8657 insertions. When your would-be reviewers pull up the diffs, they run away - this pull request is going to take 4 hours to review and they don't have 4 hours right now. They'll get to it later, just as soon as they have more free time (ha!).
Or, it could be related to best practices.
One common issue is that the pull request is too big to review.
Let's say you've touched 39 files and have 8657 insertions.
When your would-be reviewers pull up the diffs, they run away - this pull request is going to take 4 hours to review and they don't have 4 hours right now.
They'll get to it later, just as soon as they have more free time (ha!).
There is a detailed rundown of best practices, including how to avoid too-lengthy pull requests, in the next section.
But, if you've already followed the best practices and you still aren't getting any pull request love, here are some
things you can do to move the process along:
But, if you've already followed the best practices and you still aren't getting any pull request love, here are some things you can do to move the process along:
* Make sure that your pull request has an assigned reviewer (assignee in GitHub). If not, reply to the pull request comment stream asking for a reviewer to be assigned. This is done via a [bot command](https://prow.k8s.io/command-help) (the bot may have suggestions for this) and looks like this: `/assign @username`.
@ -205,58 +149,87 @@ Read on to learn more about how to get faster reviews by following best practice
Most of this section is not specific to Kubernetes, but it's good to keep these best practices in mind when you're making a pull request.
You've just had a brilliant idea on how to make Kubernetes better. Let's call that idea Feature-X. Feature-X is not even that complicated. You have a pretty good idea of how to implement it. You jump in and implement it, fixing a bunch of stuff along the way. You send your pull request - this is awesome! And it sits. And sits. A week goes by and nobody reviews it. Finally, someone offers a few comments, which you fix up and wait for more review. And you wait. Another week or two go by. This is horrible.
You've just had a brilliant idea on how to make Kubernetes better.
Let's call that idea Feature-X.
Feature-X is not even that complicated.
You have a pretty good idea of how to implement it.
You jump in and implement it, fixing a bunch of stuff along the way.
You send your pull request - this is awesome!
And it sits.
And sits.
A week goes by and nobody reviews it.
Finally, someone offers a few comments, which you fix up and wait for more review.And you wait.
Another week or two go by. This is horrible.
Let's talk about best practices so your pull request gets reviewed quickly.
## 0. Familiarize yourself with project conventions
## Familiarize yourself with project conventions
* [Development guide](/contributors/devel/development.md)
* [Coding conventions](../guide/coding-conventions.md)
* [API conventions](/contributors/devel/sig-architecture/api-conventions.md)
* [Kubectl conventions](/contributors/devel/sig-cli/kubectl-conventions.md)
## 1. Is the feature wanted? File a Kubernetes Enhancement Proposal
Are you sure Feature-X is something the Kubernetes team wants or will accept? Is it implemented to fit with other changes in flight? Are you willing to bet a few days or weeks of work on it?
## Is the feature wanted? File a Kubernetes Enhancement Proposal
Are you sure Feature-X is something the Kubernetes team wants or will accept?
Is it implemented to fit with other changes in flight?
Are you willing to bet a few days or weeks of work on it?
It's better to get confirmation beforehand.
When you want to make a large or otherwise significant change, you should follow the [Kubernetes Enhancement Proposal process](https://git.k8s.io/enhancements/keps/0001-kubernetes-enhancement-proposal-process.md).
Even for small changes, it is often a good idea to gather feedback on an issue you filed, or even simply ask in the appropriate SIG's Slack channel to invite discussion and feedback from code owners. Here's a [list of SIGs](/sig-list.md).
Even for small changes, it is often a good idea to gather feedback on an issue you filed, or even simply ask in the appropriate SIG's Slack channel to invite discussion and feedback from code owners.
Here's a [list of SIGs](/sig-list.md), this includes their public meetings.
## KISS, YAGNI, MVP, etc.
## 2. Smaller Is Better: Small Commits, Small Pull Requests
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.
## Smaller Is Better: Small Commits, Small Pull Requests
Small commits and small pull requests get reviewed faster and are more likely to be correct than big ones.
Attention is a scarce resource. If your pull request takes 60 minutes to review, the reviewer's eye for detail is not as keen in the last 30 minutes as it was in the first. It might not get reviewed at all if it requires a large continuous block of time from the reviewer.
Attention is a scarce resource.
If your pull request takes 60 minutes to review, the reviewer's eye for detail is not as keen in the last 30 minutes as it was in the first.
It might not get reviewed at all if it requires a large continuous block of time from the reviewer.
**Breaking up commits**
Break up your pull request into multiple commits, at logical break points.
Making a series of discrete commits is a powerful way to express the evolution of an idea or the
different ideas that make up a single feature. Strive to group logically distinct ideas into separate commits.
Making a series of discrete commits is a powerful way to express the evolution of an idea or the different ideas that make up a single feature.
Strive to group logically distinct ideas into separate commits.
For example, if you found that Feature-X needed some prefactoring to fit in, make a commit that JUST does that prefactoring. Then make a new commit for Feature-X.
For example, if you found that Feature-X needed some prefactoring to fit in, make a commit that JUST does that prefactoring.
Then make a new commit for Feature-X.
Strike a balance with the number of commits. A pull request with 25 commits is still very cumbersome to review, so use
judgment.
Strike a balance with the number of commits.
A pull request with 25 commits is still very cumbersome to review, so use your best judgment.
**Breaking up Pull Requests**
Or, going back to our prefactoring example, you could also fork a new branch, do the prefactoring there and send a pull request for that. If you can extract whole ideas from your pull request and send those as pull requests of their own, you can avoid the painful problem of continually rebasing.
Or, going back to our prefactoring example, you could also fork a new branch, do the prefactoring there and send a pull request for that.
If you can extract whole ideas from your pull request and send those as pull requests of their own, you can avoid the painful problem of continually rebasing.
Kubernetes is a fast-moving codebase - lock in your changes ASAP with your small pull request, and make merges be someone else's problem.
Multiple small pull requests are often better than multiple commits. Don't worry about flooding us with pull requests. We'd rather have 100 small, obvious pull requests than 10 unreviewable monoliths.
Multiple small pull requests are often better than multiple commits.
Don't worry about flooding us with pull requests. We'd rather have 100 small,obvious pull requests than 10 unreviewable monoliths.
We want every pull request to be useful on its own, so use your best judgment on what should be a pull request vs. a commit.
As a rule of thumb, if your pull request is directly related to Feature-X and nothing else, it should probably be part of the Feature-X pull request. If you can explain why you are doing seemingly no-op work ("it makes the Feature-X change easier, I promise") we'll probably be OK with it. If you can imagine someone finding value independently of Feature-X, try it as a pull request. (Do not link pull requests by `#` in a commit description, because GitHub creates lots of spam. Instead, reference other pull requests via the pull request your commit is in.)
As a rule of thumb, if your pull request is directly related to Feature-X and nothing else, it should probably be part of the Feature-X pull request.
If you can explain why you are doing seemingly no-op work ("it makes the Feature-X change easier, I promise") we'll probably be OK with it.
If you can imagine someone finding value independently of Feature-X, try it as a pull request.
(Do not link pull requests by `#` in a commit description, because GitHub creates lots of spam.
Instead, reference other pull requests via the pull request your commit is in.)
## 3. Open a Different Pull Request for Fixes and Generic Features
## Open a Different Pull Request for Fixes and Generic Features
**Put changes that are unrelated to your feature into a different pull request.**
@ -266,12 +239,17 @@ You absolutely should fix those things (or at least file issues, please) - but n
**Look for opportunities to pull out generic features.**
For example, if you find yourself touching a lot of modules, think about the dependencies you are introducing between packages. Can some of what you're doing be made more generic and moved up and out of the Feature-X package? Do you need to use a function or type from an otherwise unrelated package? If so, promote! We have places for hosting more generic code.
For example, if you find yourself touching a lot of modules, think about the dependencies you are introducing between packages.
Can some of what you're doing be made more generic and moved up and out of the Feature-X package?
Do you need to use a function or type from an otherwise unrelated package?
If so, promote!
We have places for hosting more generic code.
Likewise, if Feature-X is similar in form to Feature-W which was checked in last month, and you're duplicating some tricky stuff from Feature-W, consider prefactoring the core logic out and using it in both Feature-W and
Feature-X. (Do that in its own commit or pull request, please.)
Feature-X.
(Do that in its own commit or pull request, please.)
## 4. Comments Matter
## Comments Matter
In your code, if someone might not understand why you did something (or you won't remember why later), comment it. Many code-review comments are about this exact issue.
@ -279,17 +257,21 @@ If you think there's something pretty obvious that we could follow up on, add a
Read up on [GoDoc](https://blog.golang.org/godoc-documenting-go-code) - follow those general rules for comments.
## 5. Test
## Test
Nothing is more frustrating than starting a review, only to find that the tests are inadequate or absent. Very few pull requests can touch the code and NOT touch tests.
Nothing is more frustrating than starting a review, only to find that the tests are inadequate or absent.
Very few pull requests can touch the code and NOT touch tests.
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.
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.
## 6. Squashing
## Squashing
Your reviewer has finally sent you feedback on Feature-X.
Make the fixups, and don't squash yet. Put them in a new commit, and re-push. That way your reviewer can look at the new commit on its own, which is much faster than starting over.
Make the fixups, and don't squash yet.
Put them in a new commit, and re-push.
That way your reviewer can look at the new commit on its own, which is much faster than starting over.
We might still ask you to clean up your commits at the very end for the sake of a more readable history, but don't do this until asked: typically at the point where the pull request would otherwise be tagged `LGTM`.
@ -301,31 +283,30 @@ For more information, see [squash commits](./github-workflow.md#squash-commits).
* Sausage => squash
Do squash when there are several commits to fix bugs in the original commit(s), address reviewer feedback, etc. Really we only want to see the end state and commit message for the whole pull request.
Do squash when there are several commits to fix bugs in the original commit(s), address reviewer feedback, etc.
Really we only want to see the end state and commit message for the whole pull request.
* Layers => don't squash
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.
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.
## 7. Commit Message Guidelines
## Commit Message Guidelines
PR comments are not represented in the commit history. Commits and their commit
messages are the _"permanent record"_ of the changes being done in your PR and
their commit messages should accurately describe both _what_ and _why_ it is
being done.
PR comments are not represented in the commit history.
Commits and their commit messages are the _"permanent record"_ of the changes being done in your PR and their commit messages should accurately describe both _what_ and _why_ it is being done.
Commit messages are comprised of two parts; the subject and the body.
The subject is the first line of the commit message and is often the only part
that is needed for small or trivial changes. Those may be done as "one liners"
with the `git commit -m` or the `--message` flag, but only if the what and
especially why can be fully described in that few words.
that is needed for small or trivial changes.
Those may be done as "one liners" with the `git commit -m` or the `--message` flag, but only if the what and especially why can be fully described in that few words.
The commit message body is the portion of text below the subject when you run
`git commit` without the `-m` flag which will open the commit message for editing
in your [preferred editor]. Typing a few further sentences of clarification is
a useful investment in time both for your reviews and overall later project
maintenance.
in your [preferred editor].
Typing a few further sentences of clarification is a useful investment in time both for your reviews and overall later project maintenance.
```
This is the commit message subject
@ -344,8 +325,8 @@ Some more text
#
```
Use these guidelines below to help craft a well formatted commit message. These
can be largely attributed to the previous work of [Chris Beams], [Tim Pope],
Use these guidelines below to help craft a well formatted commit message.
These can be largely attributed to the previous work of [Chris Beams], [Tim Pope],
[Scott Chacon] and [Ben Straub].
- [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)
@ -361,20 +342,19 @@ can be largely attributed to the previous work of [Chris Beams], [Tim Pope],
### Try to keep the subject line to 50 characters or less; do not exceed 72 characters
The 50 character limit for the commit message subject line acts as a focus to
keep the message summary as concise as possible. It should be just enough to
describe what is being done.
keep the message summary as concise as possible.
It should be just enough to describe what is being done.
The hard limit of 72 characters is to align with the max body size. When viewing
the history of a repository with `git log`, git will pad the body text with
additional blank spaces. Wrapping the width at 72 characters ensures the body
text will be centered and easily viewable on an 80-column terminal.
The hard limit of 72 characters is to align with the max body size.
When viewing the history of a repository with `git log`, git will pad the body text with additional blank spaces.
Wrapping the width at 72 characters ensures the body text will be centered and easily viewable on an 80-column terminal.
<!-- omit in toc -->
#### Providing additional context
You can provide additional context with fewer characters by prefixing your
commit message with the [kind] or [area] that your PR is impacting. These are
commonly used labels that other members of the Kubernetes community will
commit message with the [kind] or [area] that your PR is impacting.
These are commonly used labels that other members of the Kubernetes community will
understand.
**Examples:**
@ -390,9 +370,8 @@ within the commit message body.
<!-- omit in toc -->
### The first word in the commit message subject should be capitalized unless it starts with a lowercase symbol or other identifier
The commit message subject is like an abbreviated sentence. The first word should
be capitalized unless the message begins with symbol, acronym or other identifier
such as [kind] or [area] that would regularly be lowercase.
The commit message subject is like an abbreviated sentence.
The first word should be capitalized unless the message begins with symbol, acronym or other identifier such as [kind] or [area] that would regularly be lowercase.
<!-- omit in toc -->
@ -438,18 +417,18 @@ If applied, this commit will <your subject line here>
### Add a single blank line before the commit message body
Git uses the blank line to determine which portion of the commit message is the
subject and body. Text preceding the blank line is the subject, and text
following is considered the body.
subject and body.
Text preceding the blank line is the subject, and text following is considered the body.
<!-- omit in toc -->
### Wrap the commit message body at 72 characters
The default column width for git is 80 characters. Git will pad the text of the
message body with an additional 4 spaces when viewing the git log. This would
leave you with 76 available spaces for text, however the text would be "lop-sided".
The default column width for git is 80 characters.
Git will pad the text of the message body with an additional 4 spaces when viewing the git log.
This would leave you with 76 available spaces for text, however the text would be "lop-sided".
To center the text for better viewing, the other side is artificially padded
with the same amount of spaces, resulting in 72 usable characters per line. Think
of them as the margins in a word doc.
with the same amount of spaces, resulting in 72 usable characters per line.
Think of them as the margins in a word doc.
<!-- omit in toc -->
@ -463,7 +442,7 @@ commit message will automatically apply the `do-not-merge/invalid-commit-message
label to your PR preventing it from being merged.
[GitHub keywords] in a PR to close issues is considered a convenience item, but
can have unexpected side-effects; often closing something they shouldn't.
can have unexpected side-effects; often closing something they shouldn't.
**Blocked Keywords:**
- close
@ -488,14 +467,13 @@ will continually do so each time the PR is updated.
### Use the commit message body to explain the _what_ and _why_ of the commit
Commits and their commit messages are the _"permanent record"_ of the changes
being done in your PR. Describing why something has changed and what effects it
may have. You are providing context to both your reviewer and the next person
that has to touch your code.
being done in your PR.
Describing why something has changed and what effects it may have.
You are providing context to both your reviewer and the next person that has to touch your code.
If something is resolving a bug, or is in response to a specific issue, you can
link to it as a reference with the message body itself. These sorts of
breadcrumbs become essential when tracking down future bugs or regressions and
further help explain the _"why"_ the commit was made.
link to it as a reference with the message body itself.
These sorts of breadcrumbs become essential when tracking down future bugs or regressions and further help explain the _"why"_ the commit was made.
**Additional Resources:**
@ -504,39 +482,29 @@ further help explain the _"why"_ the commit was made.
- [What’s with the 50/72 rule? - Preslav Rachev](https://preslav.me/2015/02/21/what-s-with-the-50-72-rule/)
- [A Note About Git Commit Messages - Tim Pope](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html)
## It's OK to Push Back
## 8. KISS, YAGNI, MVP, etc.
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.
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.
## 9. It's OK to Push Back
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.
You might be overruled, but you might also prevail. We're pretty reasonable people.
You might be overruled, but you might also prevail.
We're pretty reasonable people.
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.
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.
## 10. Common Sense and Courtesy
## Common Sense and Courtesy
No document can take the place of common sense and good taste. Use your best
judgment, while you put
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.
No document can take the place of common sense and good taste.
Use your best judgment, while you put 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.
## 11. Trivial Edits
## Trivial Edits
Each incoming Pull Request needs to be reviewed, checked, and then merged.
@ -554,6 +522,82 @@ at once to that file.
* Can the file be improved further?
* Does the trivial edit greatly improve the quality of the content?
# The Testing and Merge Workflow
The Kubernetes merge workflow uses labels, applied by [commands](https://prow.k8s.io/command-help) via comments.
These will trigger actions on your pull request.
Different Kubernetes repositories may require different labels on the path to approval.
A generic explanation of how labels are used in pull requests can be found [here](/contributors/guide/owners.md#code-review-using-owners-files).
The pull request bot will also automatically apply and/or suggest labels.
_Example:_ To apply a SIG label, you would type in a comment:
```
/sig apps
```
*NOTE: For pull requests that are in progress but not ready for review,
prefix the pull request title with `WIP` or `[WIP]` and track any remaining TODOs
in a checklist in the pull request description.*
Here's the process the pull request goes through on its way from submission to merging:
1. Make the pull request
1. `@k8s-ci-robot` assigns reviewers
1. If you're **not** a member of the Kubernetes organization, a Reviewer/Kubernetes Member checks that the pull request is safe to test.
If so, they comment `/ok-to-test`.
Pull requests by Kubernetes organization [members](/community-membership.md) do not need this step. Now the pull request is considered to be trusted, and the pre-submit tests will run:
1. Automatic tests run. See the current list of tests at this [link](https://prow.k8s.io/?repo=kubernetes%2Fkubernetes&type=presubmit)
1. If tests fail, resolve issues by pushing edits to your pull request branch
1. If the failure is a flake, anyone on trusted pull requests can comment `/retest` to rerun failed tests
1. Reviewer suggests edits
1. Push edits to your pull request branch
1. Repeat the prior two steps as needed until the reviewer(s) add `/lgtm` label. The `/lgtm` label, when applied by someone listed as a `reviewer` in the corresponding project `OWNERS` file, is a signal that the code has passed review from one or more trusted reviewers for that project
1. (Optional) Some reviewers prefer that you squash commits at this step
1. Follow the bot suggestions to assign an OWNER who will add the `/approve` label to the pull request. The `/approve` label, when applied by someone listed as an `approver` in the corresponding project `OWNERS`, is a signal that the code has passed final review and is ready to be automatically merged
The behavior of Prow is configurable across projects. You should be aware of the following configurable behaviors.
* If you are listed as an `/approver` in the `OWNERS` file, an implicit `/approve` can be applied to your pull request. This can result in a merge being triggered by a `/lgtm` label. This is the configured behavior in many projects, including `kubernetes/kubernetes`. You can remove the implicit `/approve` with `/approve cancel`
* `/lgtm` can be configured so that from someone listed as both a `reviewer` and an `approver` will cause both labels to be applied. For `kubernetes/kuebernetes` and many other projects this is _not_ the default behavior, and `/lgtm` is decoupled from `/approve`
Once the tests pass and the reviewer adds the `lgtm` and `approved` labels, the pull request enters the final merge pool.
The merge pool is needed to make sure no incompatible changes have been introduced by other pull requests since the tests were last run on your pull request.
<!-- TODO: create parallel instructions for reviewers -->
[Tide](https://git.k8s.io/test-infra/prow/cmd/tide) will manage the merge pool
automatically. It uses GitHub queries to select PRs into “tide pools”,
runs as many in a batch as it can (“tide comes in”), and merges them (“tide goes out”).
1. The pull request enters the [merge pool](https://prow.k8s.io/tide)
if the merge criteria are met. The [PR dashboard](https://prow.k8s.io/pr) shows
the difference between your PR's state and the merge criteria so that you can
easily see all criteria that are not being met and address them.
1. If tests fail, resolve issues by pushing edits to your pull request branch
1. If the failure is a flake, anyone can comment `/retest` if the pull request is trusted
1. If tests pass, Tide automatically merges the pull request
That's the last step. Your pull request is now merged.
## More About `Ok-To-Test`
- The ok-to-test label is applied by org members to PRs from external contributors, it signals that the PR can be tested.
- For a Contributor, an `ok-to-test` label means the regular CI tests will be run for their PR.
- For the reviewer or the member, labelling the PR with `ok-to-test` it means a lot more:
- They need to take care if the PR is not a wastage of our `CI/CD` resources.
- Is the PR worth testing or does it need more changes before going through the `CI/CD` process?
- Is the PR getting used to run malicious code to misuse our resources ?
- An `ok-to-test` label may reduce the workload and smoothens the contributors experience as they can know if there is any failing test. If there is, you can fix the test and they don't have to wait for a long time to get a review from `maintainer/assignee`.
- There are various other factors on which labelling of `ok-to-test` depends :
- Size of PR :
- If the PR is of `size/S` or `size/M` which is just to fix a grammatical error or spelling mistake, the reviewer can trigger the `CI/CD` without having a second thought.
- If the PR is of `size/XXL` which aims at adding a new feature, a new API endpoint or any new substantial feature. There needs to other conventions & process to be followed regarding the change made. Hence it may have a slight to delay to get labelled with `ok-to-test`.
- Other org members who are not assigned to the following PR may also label `ok-to-test` , if the change is small.
- If the PR is labelled with `cncf-cla: no`, then it is better to wait before labelling `ok-to-test`.
- PRs with tag `do-not-merge/hold` or `needs-rebase` should make the appropriate changes before the PR can be labelled `ok-to-test`.
- PRs created by mistake without to meaningful change of code should not be labelled `ok-to-test` and closed.
[Chris Beams]: https://chris.beams.io/
[Tim Pope]: https://tpo.pe/

133
contributors/guide/review-guidelines.md
View File

@ -5,16 +5,7 @@ description: |
A collection of of tips to help both those looking to get their PR reviewed
and the code reviewers themselves.
---
- [Tips for getting your PR reviewed](#tips-for-getting-your-pr-reviewed)
- [Use Tokens to signal the state of your PR](#use-tokens-to-signal-the-state-of-your-pr)
- [Reduce the scope of your PR](#reduce-the-scope-of-your-pr)
- [Split your PR into logical smaller commits](#split-your-pr-into-logical-smaller-commits)
- [Squash or remove unnecessary commits](#squash-or-remove-unnecessary-commits)
- [Explain your changes (comments)](#explain-your-changes-comments)
- [Ensure you properly test your changes](#ensure-you-properly-test-your-changes)
- [Commit messages should be thorough and detailed](#commit-messages-should-be-thorough-and-detailed)
- [Pull Request comments should summarize the PR](#pull-request-comments-should-summarize-the-pr)
- [Minor changes (typos / link fixes)](#minor-changes-typos--link-fixes)
- [Tips for code reviewers](#tips-for-code-reviewers)
- [Managing time](#managing-time)
- [Block time off to review](#block-time-off-to-review)
@ -27,127 +18,10 @@ description: |
- [Checking out a PR](#checking-out-a-pr)
- [Additional Resources](#additional-resources)
## Tips for getting your PR reviewed
### Use Tokens to signal the state of your PR
Token phrases like `WIP` or `do not merge` signal to reviewers that the PR is
potentially in a state that is not complete. This can save significant time for
reviewers on prioritizing which PR should be reviewed. They can either hold off
on reviewing altogether or will review the PR with the knowledge that it is not
complete.
### Reduce the scope of your PR
Extremely large PRs take significant amounts of time to both review and evaluate
for potential upstream/downstream impacts. If it is possible to achieve the
desired goal over multiple smaller, more concise PR ("Pre-factor"), it decreases
risk and will have a greater chance of being accepted.
### Split your PR into logical smaller commits
Multiple smaller commits that contain the logical pieces of work simplify the
review process for others to review your code. These commits should be able to
stand on their own and not necessarily depend on other things.
**Example: Markdown**
A PR that applies both formatting changes and content changes. If taken together
in a single commit, it will appear to your reviewer that the entire document has
changed and will require more time to compare and separate the actual content
changes from the style changes.
If content and style changes were made in separate commits, they could be reviewed
independently for their changes.
**Example: Generated code**
A PR that has both authored code and generated code can be difficult to parse.
If the authored code and generated code are in separate commits, the generated
code can largely be ignored by the reviewer, and they can focus their time on
the authored code.
### Squash or remove unnecessary commits
Often you may rapidly commit small changes or fixes. These are even more common
when accepting suggestions through the GitHub UI where each change is done as an
independent small commit. [Squashing] these down into a logical commit will both
help your reviewer when performing a final pass, and provide for a clean git
commit history.
### Explain your changes (comments)
Comments should be used for **ANY** non-obvious changes. They should explain why
something was done and the intent behind it; leave no surprises for the next
person to view or touch your code. If you spent time debugging, testing, or
searching for prior art on a means to do something - document it in the comments.
If you leave a `TODO`, it should explain why it needs to be revisited along with
any relevant information that could help the next person looking to update that
part of the code. Consider following insertion of a `TODO` with creating a new
_help wanted_ issue, if appropriate.
This is also true for things that should **NOT** be updated or updated with great
care. It should document why it cannot be further reduced or changed. For an
example of this, see the _"[KEEP THE SPACE SHUTTLE FLYING]"_ portion of the
persistent volume controller as an example.
### Ensure you properly test your changes
Code submitted without tests will almost always be rejected at first pass. If an
area you're updating did not previously have them, adding them in your PR will
be appreciated and helps improve the overall health of the project.
If something you change has the potential to impact the performance or scalability
of the project, include a benchmark. These are essential to demonstrate the
efficacy of your change and will help in preventing potential regressions.
### Commit messages should be thorough and detailed
PR comments are not represented in the commit history. Important details
regarding your changes should be kept in your commit message.
The commit message subject line should be no more than 72 characters and
summarize the changes being made.
Use the body of the commit message to describe **WHY** the change is being made.
Describing why something has changed is more important than how. You are
providing context to both your reviewer and the next person that has to touch
your code.
For more guidance on commit messages, see our [commit message guidelines].
### Pull Request comments should summarize the PR
The GitHub pull request should contain a summary of the changes being made as
well as links to all the relevant information related to the PR itself. These
are things such as issues it closes, KEPs, and any other relevant documentation.
Accurately filling out the issue template and applying the necessary labels
will greatly speed up the initial triage of your PR.
If there is a portion of your PR that should be explained or linked that is not
committed as a comment, you can review your own PR, applying additional context
or links.
### Minor changes (typos / link fixes)
While these sort of fixes are appreciated, try and fix multiple instances at the
same time if possible, instead of issuing subsequent individual PRs for each fix.
---
## Tips for code reviewers
If you're looking for tips for preparing your PR for review check out the [Pull Requests] page, this page is for reviewers.
### Managing time
#### Block time off to review
@ -266,6 +140,7 @@ git checkout foo
[Pull Requests]: ./pull-requests.md
[squashing]: ./github-workflow.md#squash-commits
[KEEP THE SPACE SHUTTLE FLYING]: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/volume/persistentvolume/pv_controller.go#L57-L117
[commit message guidelines]: ./pull-requests.md#7-commit-message-guidelines