Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Code Review in Kubernetes

Tim Hockin
October 12, 2022

Code Review in Kubernetes

Tim Hockin

October 12, 2022
Tweet

More Decks by Tim Hockin

Other Decks in Technology

Transcript

  1. Kubernetes has become a phenomenon • One of the most

    active OSS projects EVAR! • Hundreds of repos on github • Contributors from every timezone and almost every continent (Antarcticans - if you’re out there, holla!) We receive scores to hundreds of Pull Requests per day Every single PR needs someone to review it! Why do I care?
  2. Read the associated bugs or KEP(s) • We really do

    not want major changes without “doing the homework” • KEPs are how we think through the hard problems • There are very few non-hard problems left Understand WHY the change is being made • ...and why it is being made this particular way Before starting
  3. Read and understand the change Understand the context: the surrounding

    code and adjacent features matter a lot Think about: • Test cases • APIs • Comments and docs • Maintainability over time Start the hard work
  4. Is the bug cited? Is the bug real and worth

    fixing? Does this actually fix it? Does this prove that it fixes it (tests, benchmarks)? Is this the “right” fix? If not - it’s OK to push back Motivation: Bug fix
  5. Do we really want the feature? How does it interact

    with other features? Is the community (SIGs) OK with the design? Where’s the KEP? Motivation: Feature
  6. Is this worth cleaning up? Is this actually cleaner? Does

    this make future changes or other PRs harder? If not - it’s OK to say “no, thanks” Motivation: Cleanup
  7. Is it the simplest way to express the solution? Is

    the code as clear as possible? Do these changes fit the mission of the context? Can the change be broken down any further? • Smaller, focused PRs are better Is it a complete solution (don’t merge half-complete)? Is it exposing details that callers don’t need to know? • Can it be pre-factored to reduce scope? Clarity, cohesion, scope
  8. If code does ANYTHING non-obvious, it needs to explain WHY

    Does the code handle errors? Think about corner-cases and “pathological” situations • Think antagonistically - how will someone abuse this? Does it fail gracefully? • Will it retry? Is this clear in the code and comments? • How will the user know something failed? Comments and errors
  9. Does it log significant actions and errors? • Are the

    logs useful and/or actionable or spammy? If the PR involves config fields, APIs, CLIs, or flags: • Think about how they will be used: do they feel natural? • Are they consistent with other instances? • Would they impede or confuse future work? • Principle Of Least Surprise • If we were starting it all over, what we would want? Observability and UX
  10. Almost all code changes require test changes • Any code

    PR without tests is an immediate smell • Even if tests didn’t exist before, we need to do better Many PRs require knowledge of the context • What has been done in the past • Why things were done the way they were • How other features will interact with this change • What we want to do in the future Testing and context
  11. If you need context you do not have: GO GET

    IT! • Go exploring - software archaeology is fun • git grep is your friend • Do experiments or patch in the change and try it • Ask for a consult • Ask PR author to document the results in comments • links to previous issues / changes / discussions are VERY helpful for future others I’ve seen too many PRs approved without sufficient context that need to be fixed later • I have been guilty of this, too Domain specific knowledge
  12. If you don’t know how a change interacts with other

    code or features, or how it fits into the subsystem: DO NOT APPROVE! Domain specific knowledge
  13. Like refactoring, but done BEFORE making changes • Arranges code

    to better accommodate a future change • Reduces the size and scope of the real change • Should be no-op code changes, plus tests Prefactoring is not just acceptable - encourage it! • Any significant change will almost certainly require it • Can be separate PRs or commits in a larger PR Prefactoring
  14. PRs are not really represented in git history, but commits

    are the permanent record! PR comments should summarize • Explain what the PR is about and why • Link to relevant docs (e.g. KEPs) and bugs Commit comments should be thorough and detailed • Enough for git log to be useful • WHY > HOW PR and commit comments
  15. DO: Ask questions and seek to understand • “Am I

    following this correctly?” • “Why was this choice made?” • If you send every comment you write, you are probably doing it wrong DO: Explain your thinking • “If X is true, then Y can’t be true, but this doesn’t handle that case” Communication
  16. DO: Be clear about nits vs. requirements vs. questions •

    Nit: A small thing that could be improved • Required: “I will not /lgtm until you fix this” DON’T: Pick a bunch of nits if the real problem is that the PR has the wrong approach • Architectural guidance should be first priority Requesting changes
  17. DO: Indicate progress • Be clear about whether you’re finished

    reviewing or not, or what state you think the PR is in overall • It’s frustrating to get batch after batch of comments when you thought you were done DON’T: Approve because of deadlines or guilt • There’s always another release • Again: There’s. Always. Another. Release. Progress
  18. DON’T: Be timid about asking for changes • You are

    the defenders of this codebase • THIS IS YOUR JOB (for the moment) DO: Be reasonable • They won’t do everything exactly how you would have DO: Be willing to be wrong; trust, but verify DO: Be willing to acquiesce on reasonable debate Discussion
  19. DO NOT BE A RUBBER STAMP I’d rather we slow

    the whole project down than merge garbage
  20. TO PR AUTHORS Code review time is exponential to the

    PR size Smaller PRs have many more chances to schedule!