-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Writing Good Pull Requests
One of the biggest bottlenecks we have in Istio is PR reviews. By creating good PRs, you can help the reviewers go through your PR easily and get it checked-in quickly. This is a set of guidelines for creating good pull requests.
- Communicate beforehand
- Add a good explanation
- Keep it short
- Organize into commits
- Add tests!
- Tracking future work
It's awful when a reviewer rejects your PR, or objects to your design. You now have to throw away your carefully crafted and authored PR and start over.
If you communicate your intent for the change to the reviewer beforehand and agree on the design, there is much less of a chance of outright rejection of PRs, or substantial change requests. It also gives an opportunity for the reviewer to think about the problem and think about how the system behavior would change, before they start reviewing the code.
Unless the PR is trivial, it is a good idea to open a bug to track the issue first. Especially if this is for fixing a bug. This allows capturing more detailed analysis (and repro steps if this is a bug) separately. It can also be used to track multiple PRs against the same problem.
A good way to communicate before investing too much time is to create a "Work-in-progress" PR and share it with your reviewers. The standard way of doing this is to add a "WIP:" prefix in your PR's title. This will let people looking at your PR know that it is not well baked yet. Our infrastructure also understands that the PR is not ready for merging yet, and will not allow accidental merging.
If you just write a cryptic title and nothing else, there is not much to go with for the reviewer. The reviewer will need to reconstruct what you're trying to accomplish from your code, which is not an easy task.
Writing a good, short, to the point explanation of what is going in your commits is extremely useful for the reviewer. If there are multiple things going on (i.e. a needed refactoring, a trivial bug fix you happened to catch along the way, an issue that you found for which you're adding a TODO for), add these as bullet-points in your PR description.
This not only helps the reviewer, but also people that are looking at the repo history, trying to figure out what has changed in a particular pull request.
Do:
Subject: Fix bug(#449) that causes Foo component to crash when flag is not set.
Description:
This is caused by an off-by-one failure during iteration of nukes to launch.
Also fixed a race condition by adding a lock on the trigger mechanism that caused concurrent launches that caused a crash in the silo.
Adding a TODO for refactoring the code as well, as the cold war is over and we don't need this particular
defense mechanism anymore.
Don't
Subject: Fix minor bug.
Description:
The shorter the PR, the easier to review. Reviewing a PR requires the reviewer to understand how the system behavior is being changed. With bigger PRs, this becomes harder to understand, especially with the diff based nature of the review tools.
Keep your PRs as short as possible. A good rule of thumb is that if you PR ends up touching more than 500 lines, you should considering breaking it up into smaller PRs.
If there are refactorings that you've decided to do along the way, move them to a separate PR so that real changes aren't mingled with no-brainer refactoring changes.
If you must merge large changes, all in one go, then consider splitting your changes into multiple commits within the same pull request. This allows the reviewer to compartmentalize your changes and review them in isolation.
When making changes that are requested by your reviewers, add them as additional commits, instead of squashing it into the original. This allows the reviewer to quickly spot that his feedback is being incorporated.
Whatever the issue you're fixing, add tests! Adding tests are the best way to convince the reviewer that what you're doing actually works. It also makes sure that the product will not regress and the issue will not occur again.
It is crucial to add the right type of tests. If the issue you are fixing is due to a particular library (e.g. cache library race condition causing cache poisoning), it is perfectly reasonable to write a small unit-test to avoid the regression.
However, if the issue is larger scoped (e.g. change in product behavior), then it is important to add the right integration or end-to-end test to verify the behavior.
Sometimes in the course of a PR review, a reviewer will point out more work that should be done as part of the change. When this happens, it is common for the author of the PR to say "Good idea, I'll do that in a follow-up PR". As part of this, it's generally desirable for the author to open a new issue (assuming there isn't one already) to track this extra work. The issue # should be included in the original PR so that the reviewer can rest assured that the work will not be forgotten.
Visit istio.io to learn how to use Istio.
- Preparing for Development Mac
- Preparing for Development Linux
- Troubleshooting Development Environment
- Repository Map
- GitHub Workflow
- Github Gmail Filters
- Using the Code Base
- Developing with Minikube
- Remote Debugging
- Verify your Docker Environment
- Istio Test Framework
- Working with Prow
- Test Grid
- Code Coverage FAQ
- Writing Good Integration Tests
- Test Flakes
- Release Manager Expectations
- Preparing Istio Releases
- 1.5 Release Information
- 1.6 Release Information
- 1.7 Release Information
- 1.8 Release Information
- 1.9 Release Information
- 1.10 Release Information
- 1.11 Release Information
- 1.12 Release Information
- 1.13 Release Information
- 1.14 Release Information
- 1.15 Release Information
- 1.16 Release Information
- 1.17 Release Information
- 1.18 Release Information
- 1.19 Release Information
- 1.20 Release Information
- 1.21 Release Information
- 1.22 Release Information
- 1.23 Release Information
- 1.24 Release Information
- Collecting Logs and Debug Info
- Dependency FAQ
- Working with discuss.istio.io
- Developing with and hosting upon OpenShift
- Adapter Dev Guide
- Adapter Walkthrough
- Attribute Generating Adapter Walkthrough
- Route Directive Adapter Development Guide
- Out of Tree Adapter Walkthrough
- Running a Local Instance
- Template Dev Guide
- Using a Custom Adapter
- Publishing Adapters and Templates to istio.io
- Enabling Envoy Authorization Service and gRPC Access Log Service With Mixer