diff --git a/faster_reviews.md b/faster_reviews.md new file mode 100644 index 000000000..142ac946a --- /dev/null +++ b/faster_reviews.md @@ -0,0 +1,177 @@ +# How to get faster PR reviews + +Most of what is written here is not at all specific to Kubernetes, but it bears +being written down in the hope that it will occasionally remind people of "best +practices" around code reviews. + +You've just had a brilliant idea on how to make Kubernetes better. Let's call +that idea "FeatureX". 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 PR - 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 goes by. This is horrible. + +What went wrong? One particular problem that comes up frequently is this - your +PR is too big to review. You've touched 39 files and have 8657 insertions. +When your would-be reviewers pull up the diffs they run away - this PR 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!). + +Let's talk about how to avoid this. + +## 1. Don't build a cathedral in one PR + +Are you sure FeatureX is something the Kubernetes team wants or will accept, or +that it is implemented to fit with other changes in flight? Are you willing to +bet a few days or weeks of work on it? If you have any doubt at all about the +usefulness of your feature or the design - make a proposal doc or a sketch PR +or both. Write or code up just enough to express the idea and the design and +why you made those choices, then get feedback on this. Now, when we ask you to +change a bunch of facets of the design, you don't have to re-write it all. + +## 2. Smaller diffs are exponentially better + +Small PRs get reviewed faster and are more likely to be correct than big ones. +Let's face it - attention wanes over time. If your PR takes 60 minutes to +review, I almost guarantee that the reviewer's eye for details is not as keen in +the last 30 minutes as it was in the first. This leads to multiple rounds of +review when one might have sufficed. In some cases the review is delayed in its +entirety by the need for a large contiguous block of time to sit and read your +code. + +Whenever possible, break up your PRs into multiple 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. There's a balance to be struck, +obviously. If your commits are too small they become more cumbersome to deal +with. Strive to group logically distinct ideas into commits. + +For example, if you found that FeatureX needed some "prefactoring" to fit in, +make a commit that JUST does that prefactoring. Then make a new commit for +FeatureX. Don't lump unrelated things together just because you didn't think +about prefactoring. If you need to, fork a new branch, do the prefactoring +there and send a PR for that. If you can explain why you are doing seemingly +no-op work ("it makes the FeatureX change easier, I promise") we'll probably be +OK with it. + +Obviously, a PR with 25 commits is still very cumbersome to review, so use +common sense. + +## 3. Multiple small PRs are often better than multiple commits + +If you can extract whole ideas from your PR and send those as PRs of their own, +you can avoid the painful problem of continually rebasing. Kubernetes is a +fast-moving codebase - lock in your changes ASAP, and make merges be someone +else's problem. + +Obviously, we want every PR to be useful on its own, so you'll have to use +common sense in deciding what can be a PR vs what should be a commit in a larger +PR. Rule of thumb - if this commit or set of commits is directly related to +FeatureX and nothing else, it should probably be part of the FeatureX PR. If +you can plausibly imagine someone finding value in this commit outside of +FeatureX, try it as a PR. + +Don't worry about flooding us with PRs. We'd rather have 100 small, obvious PRs +than 10 unreviewable monoliths. + +## 4. Don't rename, reformat, comment, etc in the same PR + +Often, as you are implementing FeatureX, you find things that are just wrong. +Bad comments, poorly named functions, bad structure, weak type-safety. You +should absolutely fix those things (or at least file issues, please) - but not +in this PR. See the above points - break unrelated changes out into different +PRs or commits. Otherwise your diff will have WAY too many changes, and your +reviewer won't see the forest because of all the trees. + +## 5. Comments matter + +Read up on GoDoc - follow those general rules. If you're writing code and you +think there is any possible chance that someone might not understand why you did +something (or that you won't remember what you yourself did), comment it. If +you think there's something pretty obvious that we could follow up on, add a +TODO. Many code-review comments are about this exact issue. + +## 5. Tests are almost always required + +Nothing is more frustrating than doing a review, only to find that the tests are +inadequate or even entirely absent. Very few PRs can touch code and NOT touch +tests. If you don't know how to test FeatureX - ask! We'll be happy to help +you design things for easy testing or to suggest appropriate test cases. + +## 6. Look for opportunities to generify + +If you find yourself writing something that touches a lot of modules, think hard +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 FeatureX package? +Do you need to use a function or type from an otherwise unrelated package? If +so, promote! We have places specifically for hosting more generic code. + +Likewise if FeatureX is similar in form to FeatureW which was checked in last +month and it happens to exactly duplicate some tricky stuff from FeatureW, +consider prefactoring core logic out and using it in both FeatureW and FeatureX. +But do that in a different commit or PR, please. + +## 7. Fix feedback in a new commit + +Your reviewer has finally sent you some feedback on FeatureX. You make a bunch +of changes and ... what? You could patch those into your commits with git +"squash" or "fixup" logic. But that makes your changes hard to verify. Unless +your whole PR is pretty trivial, you should instead put your fixups into a new +commit and re-push. Your reviewer can then look at that commit on its own - so +much faster to review than starting over. + +We might still ask you to squash commits at the very end, for the sake of a clean +history. + +## 8. KISS, YAGNI, MVP, etc + +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 +features "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. Push back + +We understand that it is hard to imagine, but sometimes we make mistakes. It's +OK to push back on changes requested during a review. If you have a good reason +for doing something a certain way, you are absolutley allowed to debate the +merits of a requested change. You might be overruled, but you might also +prevail. We're mostly pretty reasonable people. Mostly. + +## 10. I'm still getting stalled - help?! + +So, you've done all that and you still aren't getting any PR love? Here's some +things you can do that might help kick a stalled process along: + + * Make sure that your PR has an assigned reviewer (assignee in GitHub). If + this is not the case, reply to the PR comment stream asking for one to be + assigned. + + * Ping the assignee (@username) on the PR comment stream asking for an + estimate of when they can get to it. + + * Ping the assigneed by email (many of us have email addresses that are well + published or are the same as our GitHub handle @google.com or @redhat.com). + +If you think you have fixed all the issues in a round of review, and you haven't +heard back, you should ping the reviewer (assignee) on the comment stream with a +"please take another look" (PTAL) or similar comment indicating you are done and +you think it is ready for re-review. In fact, this is probably a good habit for +all PRs. + +One phenomenon of open-source projects (where anyone can comment on any issue) +is the dog-pile - your PR 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 PR to clear out all the comments. +Remember: you don't HAVE to fix every issue raised by every person who feels +like commenting, but you should at least answer reasonable comments with an +explanation. + +## Final: Use common sense + +Obviously, none of these points are hard rules. There is no document that can +take the place of common sense and good taste. Use your best judgement, but put +a bit of thought into how your work can be made easier to review. If you do +these things your PRs will flow much more easily. +