Kubernetes Kubernetes Contributor Summit - NA (San Diego) 2019, 27 Nov 2019

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Contributor Summit NA 2019: Keeping the Bar High - How to be a bad ass Code Reviewer

Description

Practical techniques and approaches to code review to help keep the quality bar as high as we need it to be.

Event link: https://events19.linuxfoundation.org/events/kubernetes-contributor-summit-north-america-2019/
Session link: https://sched.co/VutA

A

Hello, contributor, summit., sorry for the chaos. uh,.

A

Google decided today was a good time to upgrade my bias.

A

Uh,, I'm tim,. I know a lot of you here. for those of you who don't know me,.

A

I've been part of the project for a long time. uh,.

A

I have the privilege of getting to look at a lot of the prs that you guys send.

A

In and give them some code, review.

A

And as we were getting ready for the contributor summit, today,.

A

We were thinking about like what sorts of interesting talks we'd go on as our, as our community is getting much, much bigger.. What are the things that we're experiencing pain with? and I said, you know,.

A

One of the problems that I have is I get a lot of code reviews that have already.

A

Been through other code reviewers, and I still find things that I think we could.

A

Do better, right? and I said, so how,?

A

How can we work on making everybody here a better code, reviewer?.

A

So that spawned the idea for talking a little bit about how I approach code.

A

Review and I had some of my colleagues look at it and give me some great.

A

Feedback on it., so I would like to share with you some of the things that I think about when I'm.

A

Doing code reviews and try to explain why and how I go about those things so.

A

That you can ingest those things so that the more you do, them,, the less I have to write.. This is purely self-serving.

A

So that said,, why do you care.

A

Just this for one second.

A

Kubernete's you might've heard of it., so a little little open source project.

A

It's kind of become this phenomenal thing,, almost unique in the world.

A

We have hundreds of repos on github., we have contributors from every single time, zone.

A

And as far as I know,, we don't have anybody from antarctica,, but if you're out there,, please let me know cause. I really would like to update these slides.

A

We get dozens of pull requests every single day across all these different repos.

A

And some of the repos or code, and some of the repos are not code,, but I'm going to focus mostly on code,, but really every single one of those pull requests needs a human to look at it,.

A

Think about it, figure out how it integrates with the rest of the system,.

A

And give feedback to the, to the author on how to make these things better.

A

As we scale out,, we need more and more people to be able to do this more effectively.

A

My goal is to share how I personally think about it and I'm totally open to.

A

Questions. I've got a lot of material here, I'll burn through it,.

A

But if people have questions that are in real time, I'd be happy to do. Those.

A

So before we get started, when you're gonna sit down and you're going to do a code, review, you've just received this email that says you've been assigned to pull,.

A

Request them from the pr bot, right? we've got these wonderful mechanics.

A

First, you've got to make the time for code reviewing.

A

This is something that I fail at a ton, right?

A

anybody who's sent me a pull request and headed languish for a long time,.

A

I know some of you are in here.

A

Part of the problem is finding enough time to do code reviews, so you have to.

A

Make the time. so I've now started. Personally, I put time on my calendar,.

A

Big red block on my calendar, it says, do not book code, review., right?,.

A

And then I'm going to use that time to go through whatever's in my code review queue., it sometimes works and sometimes it doesn't work.. But as a code reviewer,.

A

If,, if you can't make the real time to do a code review,, you probably shouldn't even start on it, right? cause a bad code review is actually maybe worse than no code. Review. right?.

A

False sense of security., you get yourself into the mindset.

A

For me, that means closing slack, closing other things and, like all the distractions that are going to be more.

A

Interesting. and then I go off and I read,.

A

I usually spend the first five to 10 minutes of any of any code review session.

A

Just reading the bugs that are associated with this pull request because of.

A

Course you have bugs for your poles, right?. I hope so.

A

I understand why, like get into the mindset of the person who's. Sending me this pr,.

A

Why are you sending it now? I can actually start, right?.

A

That was just prep work, right? crack your knuckles, roll up, your sleeves.

A

I got to sit down there. I got to,. I got to understand the change.. What do you,?

A

What are you doing?? What's going on here?

A

many times? My first instinct is to start commenting right, away, right?,.

A

Oh, this is wrong.. This variable can be better at this. Function could be moved over here, and I.

A

Don't understand what the pull request is doing yet, right?, so I've been trying now to not do that, to actually read the whole thing and understand. What's going on before I start.

A

Adding comments. now sometimes githubs. nice.

A

Now, where I can add a bunch of draft comments and I'd go back and resolve them later., I probably resolve more comments than I send.

A

And so, as I read it, I'm reading the code, but I'm also reading what's around it,.

A

Right?, where does it fit into the system and I'm thinking about, okay,.

A

When they're making this change, how, what,, what test case would prove that this case isn't completely broken.

A

I'm thinking about how do those api's fit in with the rest of the api apis.

A

Around it., I'm big on consistency., many of you, who've, been victim of my code. Reviews hear me complain about that.

A

And I'm thinking about the comments and the docs like future me is going to.

A

Really resent current me if I don't demand that I can understand the comments.

A

And the flow of what's going on, here. now,.

A

Something I really want to impart on people,.

A

Every single change that we take into our code base is going to make our code.

A

Base either better or worse., and if you can't look at a pr and see how it makes our code base better,.

A

It's the other one., and we need to be really careful at this point.

A

This project is big., it's relied on by a lot of people.

A

We can't afford to degrade the code base in significant ways.

A

So every one of us here is the front line of defense against this, and it's.

A

Really hard, right? cause here's a contributor who is offering up their sweat,.

A

Their toil to have done something that is important to them for some reason and.

A

Job is to make sure that they meet the bar..

A

They don't always know where the bar is. right?, and this is tough.

A

And for people who are in hearing contributors,, I hope maybe you can sort of understand this. As a code reviewer,, I am.

A

I personally am counting on you to get this and to hold the line on this, because.

A

It's really important.. If the code base spirals down into chaos,, we all lose.

A

So let's talk about the actual act of reviewing and the things that we go.

A

Through of you., I hope that nothing here is revolutionary and nothing here is stuff that hasn't.

A

Been written about before., but I'm going to sort of give you my spin on these things.

A

And I want to encourage everybody here: who's doing code, review. first of all,.

A

Anybody can do code review., you don't need to be a core maintainer to do code, review.

A

If you're a casual contributor and you've got a few free minutes and you see a.

A

Pull request that looks interesting, congratulations, you're, a code reviewer.

A

Anybody can do code review.. There is no limit on the number of people who can offer suggestions on how to.

A

Make code better. and in fact, I would love it if more of these prs got more crowdsource, more eyes on these,.

A

A

and I want to encourage everybody who's doing those code reviews to hold these.

A

Ideals, right?: these are the things that will keep us from utter chaos and we have to back each.

A

Other up on this, right?: this is the sort of thing where it's sort of like parents, right?.

A

And like you've got to agree on what the rules are., so I'm trying to lay out some rules for you, first,.

A

Understand the motivation, right?, what is the user doing and why not?.

A

Every bug report is a real, right?.

A

Sometimes there are bugs that are user error or are completely non reproducible.

A

Or are environmental or who knows what., so you've got to look at the bugs that they've, they've clicked through and understand the bugs and try to figure out,.

A

Sometimes I will spend time going off and reproducing the bug just to convince.

A

Myself that it actually is correct. right?.

A

How many times have I looked at a pull request or a bug and kubernetes and be like,? That's nuts.! That's not right. oh,! That's right.!.

A

So spend a little time, understand the bug.. Does this bug,.

A

Does this pr actually fix the bug or does it paper over it or does it do.

A

Something else that makes the bug less important? and importantly,.

A

Does this pr prove that it fixes the book? right?.

A

We're at the point in this project, where we can't really afford to have code.

A

Changes without tests and if,.

A

If any of these things are not there,, if these are not being held, to,.

A

It's okay to push back, right?, it's okay! To come back to our contributors and say thank you for this pull.

A

Request., this really needs a test.

A

Sometimes that test doesn't exist and they have to go off and write, be the first.

A

One to write it and that's okay. right?, like I, don't actually think we're asking for too much., mostly when we're asking people to write tests,.

A

The other kind of pr is you're going to get are the the features, right?.

A

These are the fun ones and not,. Not every feature is something we want. right?.

A

And we get a lot of this now, people who want to do something that scratches their itch..

A

This is the beauty of open source. as the maintainers,.

A

We have to understand how every feature fits together,, right?,.

A

I heard it that there is a budget of cleverness for the project and once we run.

A

Out of budget,, that's it., there's no more clever beans to spend and every feature we add, spends a couple.

A

Of those clever beans., so we have to make sure that the things that they're adding are the things that we actually want,, that we understand how it's going to interact..

A

And intersect with other features, right?.

A

How many times have we added a feature over here and broke something over there?.

A

Right? the system is big and complicated and interlinked in sometimes non-obvious ways.

A

And more importantly,, where steven, where's the cap at this point in the project?.

A

Any nontrivial change warrants a cap.

A

The cap process is not that hard.. It is not that heavyweight, right? a cap.

A

And if you find a feature request that you can't quite understand, what's what.

A

It's doing or why, ask, where's, the cap?.

A

So let's talk a little bit about sort of the style of code.. These are,.

A

These are sort of superficial things,, but they matter a lot., so clarity, right?.

A

Is this code the simplest way I can think of to express this problem, right?.

A

It's possible, if you look through the code base, close enough to find code, that.

A

Is all sorts of convoluted and twisty and could be simplified in the way it's.

A

Expressing, right?

A

we're striving for simplifying and finding the most pure way of expressing,.

A

Expressing a change., this is naming, right?.

A

Sometimes go encourages bad habits,, like naming things with one letter.

A

Sometimes that's okay, and sometimes it's really not.

A

Right code should read like I like to look at the code and understand it like a.

A

A

white space is one of those things that people tend to forget about, but actually.

A

Is really important. white space? Helps you break your code up into chapters,.

A

Right?, so it's okay for, like a pull request, comment for you to feed back.

A

Hey,, can you add a blank line between these blocks, right??

A

It would actually really help the flow or add a comment or explain the.

A

Relationship between these two thoughts. is.

A

Cohesion is the idea that a thing makes sense as a whole.

A

And so when you look at a file,, you have to ask yourself: like,.

A

Do all the functions in this file belong in this file? right?.

A

Do they make sense with each other as a team?.

A

The functions are the types that are being defined in this code. Base..

A

Do they make sense?? Do they fit the mission of this?.

A

And it's okay to ask a user, hey,! You know, this,!

A

This method doesn't really make sense on this type..

A

What we should do is make a new type that carries this method and here's how.

A

These things should be related, together, right?.

A

Those are the sorts of refactorings that will make the code better.

A

It's really bad if you're just forced fitting it in like, well,. I have the data in this type, anyway,, so I'll just jam. Another method on there.

A

It doesn't make sense. right,, but it happens. All the time.

A

The question now is: can you,, can you break a pr down both in terms of what it's doing and in the sort of the.

A

Size of it,, the girth of a, of a code review.. We have a bot now that still,.

A

That labels pull requests as they come in and every time somebody sends me an xxl pr. I die a little bit because I know that it's going to take me two hours to.

A

Review it and that's going to be a bad experience for you as a contributor. If.

A

You send me one of these giant prs., so one of the things I always push back on,.

A

Can you,, can you break this down? Right? can we,? Can we make this into two prs?.

A

Does it make sense?? Are there two thoughts here that we can extract from each other?.

A

But the opposite is also true. like I don't want to commit half cooked work.

A

Once upon a time, we thought it was okay to check the api in and then come along.

A

A few days later and checking the implementation. right?, we don't do that anymore, because it's just too much risk that that second part.

A

Never happens. and then we have an api with no implementation, and then the release comes along.

A

And the release team has to kick out prs,, but they've been touched by other prs and it's total mess.

A

So we don't do that. Anymore., we'll talk in a bit about sort of commit sequencing and commit hygiene.

A

So you've got to make your prs complete,, but you want to try to find the smallest way you can do. It.

A

And then it also goes to the sort of the scope of the code, right?.

A

Like are you exposing things that you don't need to expose??

A

Are you reaching into places that you don't need to reach into as you're.

A

Reviewing this? the code. so as a,.

A

As a code base has grown, right?, I remember package utill right?.

A

There are, there's code, that is general purpose and there's code. That is very specific.

A

Purpose and sort of the more deeply specific something is the less the api.

A

Matters and the more sort of leeway you're allowed to give people as you're.

A

Reviewing their code., if you send me a pull request against github.com kubernete's utils, you can be.

A

Darn sure I am going to pick the heck out of that api. right.

A

Cause that is a public api that we're committing to support that we are saying.

A

Is usable by people outside of the project, even. right. or by project,.

A

By sub projects across the whole org.

A

I'm going to pick the api to death and I'm going to scour it for comments and.

A

For tests and for function, signatures., something that's buried deep down under package; kubelets something,, something,.

A

Something like,, you know: what,, I'm, okay, with that being a little less general purpose.

A

A

and we heard of high rums law., I didn't know the name of it,, but I knew that the idea of it, the idea is anything that you expose through your application will become a.

A

Feature of your application. everything that's in there, somebody will use, and eventually you can't change anything because everything's.

A

Being used. so whenever you're writing an api or a module and go,.

A

You need to think about what am I exposing here and how are people going to.

A

Abuse it or use it, wrong.

A

There's the idea of make something easy to use, right and hard to use. Wrong.

A

Very much applies here.

A

So moving beyond style, there's the actual meat.

A

So maybe I'm unique in this. I write as much comment as I write code usually,.

A

And you can tell if you dig through kubernete's, you can find the code that I've written, because there's big blocks of comments that explain what I'm doing and I.

A

Got to tell you the secret., my memory is terrible..

A

I cannot remember what my own rationale was for making certain decisions.

A

And if I don't document it,, I will forget it the next time I come through and I will fix it.

A

right and future me hates past me when I don't read comments.

A

So I write a ton of comments. when I review code,.

A

I tend to ask for a lot of comments because, if you're doing anything that isn't.

A

Immediately obvious to me,, I want to know why..

A

I want that comment to explain to me why,, but not how..

A

I don't want you to explain loop over every variable in this list.

A

That doesn't make sense, right?, but if you did some analysis,.

A

If you did some testing or you scoured some, some,, some other api's,.

A

Or you did some debugging and you came to a decision.

A

Document that right?, if you wrote a benchmark document, that, if you're just picking a random number,.

A

Document that just write it down so the next person who comes along,, it doesn't wonder. Why is this value 93 by? Why isn't it zero, right?.

A

If you think that you or somebody will need to revisit a decision, right?.

A

We do this all the time, right? strategic debt, where we say,.

A

I don't really know what I'm going to do with this,, but we'll come back to it to do,, leave a comment explaining why we're coming back to it later and what you think.

A

The next person to come along needs to think about.. Here we have this amazing community of people and if you give them things to do,.

A

They will do them. right., and I have learned this over and over again.

A

If you can give people guidance on what you need them to do, they'll,.

A

They will do it. so leaving these to do comments is actually great, because community people can.

A

Come along and actually help you, right?. We can be force multipliers.

A

Mmm., if the next reader is going to come along and try to fix what you did,.

A

You need to document it. right. and we've seen these, the,.

A

There was a post a while back about the space shuttle comment,.

A

Buried it deep inside community somewhere, like the team that worked out, that.

A

Code spent days working out the flow of how this code should work.

A

And then we wrote two pages of comment explaining why the next person to come.

A

Through should not obviously fix it. right? why they were all done on purpose.

A

I'm still very proud of that. Comment.

A

So now, you're, looking at the code and you're trying to figure out what's going on., how ha how errors are handled is often a great indicator of the quality of the.

A

Overall code, right? in my experience,, so I'm looking at the errors.

A

Are you doing the right thing with errors or are you ignoring them??

A

Are you logging them and pretending they didn't happen, or are you explaining.

A

What's going on with the errors and why it's okay to proceed or why it's the.

A

Imperative that you return early?, are you thinking about the corner, cases,.

A

Right?, this is that space shuttle code again,. Are you looking at? What is the else to this? If right?, because for every,.

A

If there is an implied els is the code handling those cases,.

A

And then I like to put on my pathological user hat and think, well,.

A

What's the dumbest thing, I could possibly do with this code?

A

if I'm adding a field to an api,. What, if I pass a negative number here?.

A

Will you catch it and think through those air cases and through the errors.

A

Now those things are gonna get handled and then think through the failure. Modes.

A

Right? okay,! Well, if this thing returns,, what is the color of this function going to do with that? Error?.

A

Are they going to retry,? Are they going to abort or are they going to panic?.

A

These things are the things that tend to bite us in the butt way too late.

A

If you get these things committed and then you use your merchant user starts using it, and then you crash loop, the controller manager, for example, rob.

A

Mmm., while you're in the code,, we also have started really relying on the instrumentation that we're putting.

A

Into community. so observability, metrics and logs.

A

Are the logs good and useful and helpful, or are they just going to be spammy?.

A

Garbage, right? anybody looked at var log kubelet, like it's full of interesting information and.

A

It's really hard to find that information, because there's so much stuff.

A

Surrounding it, right? and this is,, this is just a chronic problem with organic software, right?.

A

It grows over time and logs seem important at the time and they get added and.

A

Then over time they turn out to not be useful., so it's really important to to think about. How is this log going to help the.

A

Person who has to look at the log to figure out what the heck happened?.

A

And I mentioned metrics.: we don't need metrics everywhere,.

A

But we need more metrics than we currently have.. So if you're, adding tricky,.

A

Complicated code for something,, if you have a retry loop or a counter or a timer,, think about like if this thing goes wrong,.

A

How will I know if I don't have a metric?.

A

Now,, jordan did a great review earlier on api reviews, right?.

A

A lot of prs end up touching either one of the communities api apis or the cls.

A

Like kubectl or the config fields, right?.

A

We've got a lot of the component standard stuff.. These are all ux,.

A

Right?, these are all part of what users are going to experience as they use our system..

A

So we need to think about. Do they feel right to can,? If I look at this,.

A

Does it make sense how the users are going to want to configure their system?.

A

Are they consistent?, I'm huge on consistency and sometimes like? We did the wrong thing over here and.

A

We could do the right thing and be different, or we could do kind of the wrong thing and be consistent., I'm more often going to choose the consistent but slightly wrong. Thing.

A

It depends on how wrong the first thing: was.

A

mmm., you know, the principle of least surprise, right?.

A

Once you learn how one thing works,, if you can take what you've learned and apply that to the other things,, that's really powerful, right?, and so we need to keep that in our systems.

A

And I like to ask myself if I could start over,. This is a mess.

A

If I could throw the whole thing, away, blow it up and start over,, what would I want it to look like right now?? Can I move towards that?.

A

Mmm. yeah. and as I'm looking through the code,, I'm looking at testing, right?, sometimes I got to admit it's really attractive to just sort of skip the test.

A

Files and assume that they've done the right thing., it's a really bad habit and it's a really bad idea.

A

and as somebody who has tried to pull off the, not writing very good tests. Thing,.

A

It always comes back to bite you., so it's really important as you're looking at a code change to look at the test, cases that go with it. and sometimes I'll pop open. Two separate windows and I'll actually walk through the code and walk through the test cases and make sure that the interesting.

A

Cases are being handled right.. What happens if I throw that negative number in?.

A

Does it return an error? and is there a test case that shows me that the error is properly returned?.

A

And again, some smells like as you're looking at tests,.

A

Like are you setting global variables,?

A

Are you calling global things like enabling or disabling controller manager.

A

Flags from deep inside a test or are you using time?.

A

Right? any test that uses time is an immediate smell,, right? um,.

A

I'm also looking for tests to think about ways that future me will probably want.

A

To break this code, and how can I defend against that?.

A

The tests are the only automatic signal we get right.

A

Now. I saw wojciech over there and he can give me a hard time for this.

A

Every pr that goes in is going to have some impact on performance and scale,.

A

Right? kubernetes is a highly interwoven system and our scalability actually really is.

A

Material to the success of the project.. So if you are writing a pull request and your motivation is performance or lock,.

A

Contention or scalability, or something like that,.

A

You'd better have a benchmark, right?. If you say,, I'm reducing the time on this loop,.

A

I want you to show me where's the benchmark. That shows me a and b, right?.

A

Cause a lot of the obvious optimizations aren't go, is actually really tricky.

A

This way and I've had to learn over and over again that go, does a bunch of.

A

Things cheaply that seemed like they should be expensive.

A

So those sorts of changes,, especially if they're not obvious,, they need to be well commented.

A

I'll pick on wojciech jackson's he's out there and he sent me a bunch of prs against kube proxy that changed the way all the strings are allocated. right.

A

And it makes the code more complicated and I was really sort of, ugh, really?.

A

Do I really need to do this? and he showed me the benchmark. yep,?

A

I really need to do this. right., so it like it really matters.

A

A

I've gotten through most of this pull request.. Is it complete?.

A

Do I feel, like everything that is needed here, is needed?.

A

Did the cli part of it get included in the pull request??

A

I don't really want to add things that need a cli and then not add their cli at.

A

The same time for client server, things., right. or api is and controllers.

A

We have a lot of two-part and three-part concepts in kubernete's.

A

We don't want to merge those, those half complete, pull requests.

A

I mentioned this, so now this is my favorite one.

A

Domain specific knowledge,, right? kubernete's has a lot of sub domains in it, and sometimes I get pull requests for.

A

Sub systems that I don't really know that well anymore, right?.

A

If somebody sends me a pull request for something in the scheduler,, I feel very poorly equipped to actually review it cause. I don't know the.

A

Surrounding code, anymore. right.

A

there's a whole team of people. Who've been working on it for years. in my,.

A

In my non presence,.

A

I would rather see people decline. Those pull requests than try to do them.

A

Without the appropriate domain knowledge. right., but if you're going to do them,.

A

Go get the domain knowledge, right?

A

go talk to the people who actually do own it., so I can go talk to the scheduler folks.

A

or if I have a problem that needs kubelets support,, I will go talk to the cubit people and understand what does this mean?.

A

How does this work?, what is the context of it??

A

This is really important, and I see too many pull requests that are approved by.

A

People who probably don't have the appropriate context for it.

A

And these are,, these are all bugs, right?.

A

They are the things that get in the system and come back later and you go,.

A

What happened? Here?, who did this? right?

A

and I hate my least favorite command- is get blamed, right? um,, so, like I said, go, go get experience, right?

A

it's for me. Sometimes this means spelunking through the code.

A

Get grep is my favorite command., I'm always looking for where's. This used?.

A

How is what's going on with this? who, if I returned an error from this,.

A

Who are the,? Who are my callers, right?? I will,, I do lots of experiments, right?.

A

I have my clusters up and I'm constantly just like,. This doesn't seem right.

A

Let me run a little test,. Let me throw this thing in and building the experience that I need to do a.

A

Particular pr. and as many people,.

A

Especially the long time maintainers know,, I do not hesitate to reach out and say,. Can you help me?.

A

I don't know what this is doing.. Can I get a consultation?.

A

So I encourage everybody here to feel like this., like most of us are on slack,.

A

Right?, if you're stuck on a pr and it's something that I can help you with,.

A

Let me know. um,.

A

A

If you,, if you get to a place where you're, just not sure,,.

A

Like absolutely a hundred percent, confident that this change is going to be okay,, don't approve it, right?, don't be that rubber stamp.

A

So let's talk, I guess about, commits and commit hygiene, right?.

A

We are not great at this as a project, right? and let's very,.

A

Very frankly, we're not great at commit hygiene.

A

I want to encourage everybody here to think about it as you're reviewing pull.

A

Requests to think about how do the commits that make up that pull request? Add up.

A

To the story, right?, these are the permanent records..

A

These are the things that go into get a pull request itself, isn't really meaningful in the good sense., so I am always encouraging people to take their prs and break it down into.

A

Smaller commits where each commit sort of stands on its own and is meaningful on.

A

Its own., it doesn't need to depend on the other things and they make sense in order,.

A

Right?, it makes it way easier to review a big pull request.

A

too often we see this as I did the api in one commit,.

A

I did the generated code in another commit and I wrote the implementation in a.

A

Third commit, right?: this is a really common pattern..

A

It makes get bisect hard, but it makes a review is really easy..

A

So this is a trade off that we make,, but we need to think about this right. Now.

A

One of the common things we do because github is another commit that says fixed.

A

Comments,, right? and you'd do another round review and fixed more comments and now, at the end, you've got nine commits in your pull request and the bottom five of them are all.

A

Fixed comments. one fixed comments, too.. I don't want to merge that right?.

A

Cause those, those commits are not useful right. Now.

A

You have to push back on your contributor and ask them to go and merge those.

A

Commits back into their original commits and get gives you lots of tools for.

A

This and somebody is doing a talk today on practical get for kubernetes users,.

A

Which I hope we'll talk about the idea of interactive rebase and moving commits.

A

Around the trees., it's okay to ask for this, right?.

A

And as a contributor,, if you get asked for this, like it's, not the end of the world,, it's not so hard.. There are ways to get through it,, but it actually is really important..

A

A

Something that I'm personally pretty guilty of is bad, commit comments,.

A

Right? these things are again, the permanent record.

A

So when you say get logged and you see the history of everything, that's going on,. Those commit comments are all the clues you have as to what happened and github.

A

Itself makes it a little tricky for you to go, find those individual commit.

A

Comments., you actually have to click on the separate, commits tab and go read each of the.

A

Individual commit comments. as a code reviewer,.

A

I need you to do that and demand that the commit comments actually make sense. As.

A

A contributor, I need you to think about when you're writing those comments,.

A

What do I say that explains what I did and how I did it?.

A

That leaves a clue for the person who's going to look through the get logged to understand it., there's nothing worse than having to run, get blamed on a file,.

A

Finding a line,, going, doing get log or get show on a single commit and not understanding why the heck.

A

This commit was made. right?. It's super frustrating when you're.

A

History of it,, so it's incumbent on us to push on people and make sure that we get good.

A

Comments. the, why is always more important than the how,.

A

But sometimes the how is really important too in the commit comments,.

A

Not in the code, comments and also referencing bugs is a great thing to do.

A

Right? often we do that in the pr comments.

A

those don't make it into the permanent record.

A

Pre factoring is one of my favorite things., the idea of re. We all know what refactoring is, right?.

A

But pre factoring is when you've got to change,, you want to make and you're going to move some stuff around to make it easier to.

A

Fit your change, in. right?, I'm going to add some parameters to this api or I'm going to move these functions around and I'm going to break it up and I'm going to do it in a way it.

A

Is a net, no op to the rest of the code base? That shows that the viewer can look at the.

A

Pre factoring commit and say,. This makes sense. there's nothing going on here.

A

Why are you doing this?, I will., I'm doing it, so I can make room for my next commit to come in and it'll be a much smaller change if I've done it in two steps. right?.

A

And very often I see pull requests that I have to stop and say, you know,.

A

Can you first do a commit that does all these renames and then add the thing on.

A

Top of it, instead of doing it all in one commit. renames is a classic example here and as a contributor, you should actually love.

A

It because you get more commits. everybody's counting, the points, right?.

A

So, as a reviewer's like,, it's not just acceptable..

A

It's actually encouraged like tell people to do this.

A

If you do a pr that has five pre factoring commits, and then you want to actual.

A

Commit, great., I love it.

A

Procedurally, as their code reviewer.

A

now, if somebody sends you a pr,.

A

Do ask questions, right? most of when I'm writing a comment,.

A

Anybody who's again been a victim of my pull requests,.

A

Most of what I write are questions, right?. Why did you do this?

A

what's going on here? and I actually don't want you to answer the question to me..

A

I want you to answer the question in the code, right?. If I'm asking a question,.

A

It's not obvious from the code right, now. mostly,, you know,.

A

Try to phrase the questions in a human way, right?, like am I understanding this cause maybe,. Maybe I misunderstood it., I'm kind of an idiot., so maybe I'm just not following what you're trying to do here or why did you,.

A

Why did you make this choice?? Like I said, before,.

A

If I write probably twice as many comments as I actually send, right?.

A

Cause, I start, writing the comments,. Then I get further down and I go,. Oh, I understand that now. and I can go back and close out that comment.

A

If you write,, if you send every comment, you write, you're, probably doing it wrong. and then, you know,.

A

Alpha will explain my thinking to someone like I don't,. I don't follow. This.

A

Like, cause, if x, is true, then why can't be.?

A

So why are you testing for this thing over here? When you should've checked it.

A

Way, before? and then I've explained what I want to see out of their code review or out of.

A

Their pull and they can go back and act on those things. all right.

A

As a reviewer,, you are a representative of kubernetes, right?.

A

So you need to put on your best face and b, be excellent to each other.

A

Code of conduct applies in the code reviews too.

A

It's okay to push back and for changes on a pull request. right.

A

It's actually pretty rare that a pull request comes through and it's perfect, the first time through. right.. I know when I send pull requests,.

A

I get comments all the time., it's fine., it's part of the game going back and forth and getting the thing the way you.

A

Want it to be,, but do be clear when you're doing a code review about how much you care about a.

A

Thing. right., sometimes it's a, well,. This is not really named ideally,.

A

But I guess it's okay. right?. Sometimes it's the,.

A

I really will not accept this until you make it a change. for me,.

A

It's also important to not pick all of the nits when, fundamentally the.

A

Architecture is wrong. right? because, as a, as a contributor,.

A

You have this experience of like, okay, cool.. They found 25 little things to fix, and then you get to the last one. it says,.

A

Yeah,, but actually I don't think we should do it. This way. right.

A

That's kind of obnoxious. so do look for the general architecture and pattern. First before you jump into.

A

The nitpicking. as much as it's fun to pick the nits,.

A

And sometimes when you're busy, the nits are easier to pick., then the architectural things go for the big fish first.

A

And as you're doing a code, review, sometimes,, especially if you draw me, sorry,.

A

It's a lot of back and forth, right?, and so I try to indicate progress, right?.

A

Like, okay,, I think we're almost there.. Most of my comments are relatively small at this point, so that somebody can.

A

Understand where they are in the life cycle of a pole., there's really,.

A

It's pretty frustrating when they think they've just finished everything that.

A

You've asked them to do and then you drop a whole new bomb on them. right.

A

and they have to go back and do it again. and I apologize to anybody. Who,.

A

Up to whom I've done that. I'm trying to be much more cognizant of it,.

A

But at the end of it,, like we have these deadlines, right?, this is part of our community, right?. We release on time or the train will leave without you, right?.

A

Do not approve things because you feel guilty or approve things, because the.

A

Deadline is here, and I really want you to make the train, even though you really.

A

Shouldn't. I've seen it happen.. I've actually done it.. It's terrible.

A

We shouldn't do it., there's always another release, right?.

A

It's three months: away.! That's not that far in. within a couple of weeks,.

A

The door will be open again and you can get your stuff merged.

A

Don't, don't be afraid to ask for changes., like I keep saying the same thing,.

A

But as the code reviewers,, you are empowered to ask people to make changes,.

A

To talk about the code, to understand what they're doing and why.

A

this is your job.. This is what you volunteered for by taking on a code, review,.

A

But do be reasonable. right?

A

people own their own code and they won't always do it exactly the way you think.

A

They should or the way you would have done, it. and that's okay. right?.

A

That's part of being in the community. so be reasonable when they come back with something that addresses what you.

A

Actually wanted them to do,, even if it's not exactly what you would've done.

A

Right. so in my mind,, when I'm doing a review,, I'm always thinking about,, well,.

A

What would I call this variable? or how would I structure this loop?

A

and I suggest it,, but I say, you know,, it's really,, it's yours. figure out.

A

And be willing to be wrong. right?. This is my favorite part is when I say, well,.

A

Actually, this isn't gonna work and something back and says, oh,. You missed that..

A

We set this variable up at the top of the loop and I go, right, thank you.

A

Of course. um, and, and try to be reasonable and be willing. To,, you know,.

A

If you're having a debate about something,, be willing to be wrong, and, and acquiesce, right?.

A

I think it makes for a stronger community when people can argue with each other,.

A

But if you get too adamant that you're never going to lose an argument,.

A

We will lose that culture, right?.

A

I think that's a good part of our culture right now that we can actually have.

A

Frank vigorous debate and then, when we're done with it,.

A

We're done with it, right? and it doesn't become personal, right?.

A

So you're always critiquing the code,, not the people, right?, never say,.

A

Why are you such an idiot?, I mean,, why are you doing this? The wrong way?.

A

It's why.: why does this code? Do it the wrong way??

A

That's really subtle language things that can make a huge difference.

A

Now, I talked a little bit before about tech, debt and strategic debt.

A

We're project we want to move fast.. We want to press forward all the time.

A

So sometimes we do take on debt, right?, but we want to be really strategic about taking on technical debt.

A

Most prs should not be introducing debt., they should be paying debt off.

A

If you think that there's a place where you can take a shortcut and allow it.

A

Because something really important can get done and you can come back and fix it later. then, let's talk about the trade offs and let's talk about what the risk of this.

A

Really is., you take it to your sig or the multiple stigs.

A

if you're crossing those boundaries and talk to each other,, right?.

A

We have to trust each other to make good decisions.

A

And sometimes the right decision is to press forward and get something done in a.

A

Sort of dirty gross way if we can come back and fix it in a reasonable time,.

A

Later or if the risk is low,, right?.

A

So, at the end of the day,, after all, this code review is community service,.

A

Right? and this is,, this is why we're all here, right? this is,, it's amazing.,.

A

We went to this, uh, the social last night and it was really like hanging out with hundreds of my.

A

Best friends., I think everybody in the room for being willing to come and listen to me. Talk.

A

About code review,, which is, gosh, the most exciting thing, all day,, right?.

A

Because it's really important to me- and I am super thankful- that everybody here.

A

Is willing to do this. if you don't have time,, that's okay, right?.

A

Either make the time or don't. it's fine,, but don't do it halfway.

A

We have to live in our code, base, right? for all of us, day in and day out,.

A

We're working on this thing and every time we find one of these things that you look at and your god who did this right,. It breaks my heart. A little bit,.

A

So let's,, let's try not to leave so many of those landmines. Around,.

A

But do trust your instincts, right?

A

everybody here has a sense of what's good and what's not right,.

A

And we all have a sense of smell.. We can tell when you look at something,.

A

Sometimes it just doesn't make, if it doesn't look right,.

A

Go and investigate it. now,, sometimes you'll be wrong,. Right?.

A

But you'll learn something either way. either you found a bug in the code or you found a bug in your instincts, right?

A

and you can adapt. and over time,. If you spend enough time staring at kubernetes, code, you'll start to understand the things that tend to work and the things that tend not to work. and again,. If you don't understand, it,.

A

If you're, not sure of it,, don't approve it, ask for somebody else to look at it,.

A

Right?, there's a great body of people, we've sort of got a pyramid. Going, right?.

A

If you're not sure, what's going on, reach up the pyramid and get jordan to help, you review it because jordan knows.

A

Everything, right?, but jordan can't do every pull request.

A

So if we can get the majority of people handling the majority of pull requests,.

A

Then jordans are working on the important ones, right? and the hard ones.

A

And again, just don't be a rubber stamp, right?.

A

It's super tempting to look at pull requests even from people. You know and.

A

Trust. people you work with day in and day out and just say, oh, well it's,.

A

It's,, it's frankie smart approved without even looking at it.. It's a disaster.

A

And every time I've done it and I'm guilty, I've done it.

A

Every time I've done it,, it almost always comes back to bite me in the butt.

A

Right. and it's so embarrassing when vojtech calls you up and says you broke the.

A

Scalability test,, why did you approve this pr and you go,.

A

Cause, I didn't look at it closely: enough. right.

A

And that's on me and now it's on you.

A

So to the two people who are sending prs,, I hope we have everybody in this room,.

A

Does both of these jobs, right?

A

code review is exponential to the size of the pr, right?.

A

Exponential, right., we all know what that means., the bigger your pr is,.

A

The longer it's going to take me to review it., you send me a small pr,.

A

I will probably find time to review it today or tomorrow.

A

You send me a medium pr,. I will probably find time to review it. This week.

A

You send me an xxl pr,. I will probably find time to review it. This year. right.

A

And really like I've done these xxl prs when sometimes that's the only way to do.

A

It. and they take hours,.

A

Hours and hours and hours to go through these code reviews. and on any given day,. I don't always have hours that I can spend to it.

A

So I'm encouraging you to try to break your things up and find more ways to.

A

Schedule into the, into the code review, system,.

A

The end., and because what questions do we have here- and we wants to talk to me about what.

A

They hate about code review.. What do you think about like machinery,, you know,.

B

You make maybe four lines: [inaudible].

B

You have about 5,000 lines to change., you didn't actually do it and then his version is.

B

There should be multiple commits, or do you have guidance on that?.

A

You mean like generating code?, I mean to say those should be separate, commits.

A

On specifically on generating code or on human done., but sort of mechanical things.,.

A

I often ask people to put generated code and a separate commit just cause. It's.

A

Easier to review., but I will say githubs actually gotten much better.

A

Like the tools and github in the last year have gotten significantly better.

A

Here., so that changed since last viewed feature is actually really great and it's much.

A

Easier for me to open the files view now and scan down and see how many of these.

A

Things are auto-generated and just sort of close them..

A

I don't need to review the auto-generated code cause the cis system will.

A

Regenerate it and compare and make sure that they're not wrong.

A

And so more and more, I'm actually starting to lean on actually putting in the.

A

Same commit so that things like get bisect will eventually work.

A

Yes, this is, this is challenging.. This is a place where,.

A

Sorry to repeat the question, like, if you have a bunch of things that you're going to fix, small changes that are scattered throughout the pole and the simplest thing to.

A

Do is to make a bunch of your changes and add a new commit that says fixed.

A

Comments, right? and it is really attractive and actually makes the reviews really easy, because I.

A

Can look at that last commit and say yes,. In fact, you did the things that I asked for., but that's only true at a small scale, right?.

A

If I asked you for 25 things and I see a commit that has 24 changes in it,.

A

Do I really know that you got all the things that I wanted you to get?.

A

And this is a place actually in my experience,, the tooling, around github is actually pretty lacking. Still. right?.

A

One of the things that github does that's nice and awful at the same time is if.

A

I put a comment on the line and then you go and you change the line, get hobo, make the comment, go. Away. right.!

A

Well, it's great, because I can see that there are no outstanding comments.

A

It's awful because I don't remember what I said. right?, and so what I find myself frequently doing is literally opening multiple tabs or.

A

Multiple chrome windows,, having one open with my last commit, or my last comment: review,.

A

Which actually has all my comments on them and going through each of my comments.

A

One by one and making sure that the change that I asked for is in there.

A

And at that point, the, you know,, the extra commit doesn't save me any time in reviewing because I'm looking at.

A

The overall file view anyway.

A

so, for the sake of things like get bisect and just good, commit hygiene,.

A

I prefer to go and make those fix ups in the commits that they make sense. In now,.

A

As a contributor,, especially if you have a lot of commits.

A

now, this is something I do a lot.. I don't know if everybody does this,, but I love tiny little commits, right? rename this variable, commit, add,.

A

Fix some comments, commit, move this function to the top of the file commit.

A

And now, if you say, okay, well, actually I liked, I like the comment in this function and I moved it and then I changed the common.

A

And then I renamed the thing in three separate commits..

A

I actually have to go through those commits and do that interactive rebates or I have to do the fix ups along the way.

A

I think that's part of the cost of just doing this. the commit hygiene matters.

A

If you look at other communities like the linux kernel, community,, which you know is not always a paragon of ideals,.

A

They have really good commit hygiene, right?

A

and if you look at the good history of the linux kernel,, it's very clear what happened and when and by who. right?.

A

And maybe in that respect we should strive to do better.

A

What trends have I seen for reviewers? missing, problems? great question.,.

A

I don't know that I've identified any particular trend..

A

The biggest one is reviewers approving things when they don't have the full.

A

Context, right?, it's a complicated system at this point.

A

And so I,, if I'm coming in as a domain expert on a networking thing, and I see that.

A

Somebody approved a pr that touched something in networking land but didn't.

A

Understand really what it was doing or why, that's. When I get frustrated. right?.

A

Because, like I would've rather, you called me and or slacked me and said,,.

A

Hey,, can you take a look at this? right?.

A

Because finding it later after the fact is super frustrating., so it's mostly,, it's the domain. Specific knowledge is the biggest one that I've seen.

A

Or simply not being tough enough like api is matter communities utils in.

A

Particular, like I'm ruthless about those api's.

A

Anybody who's watched me do those, those code, reviews, they're, like,,.

A

I'm going to pick the heck out of those api's, because that should be the very.

A

Best of the public apis that we're offering, and that means taking code from.

A

Kubernetes, kubernetes and moving it into communities. utils, isn't enough,.

A

Right? you taking the code,, I'm going to ask you to think about that api again,.

A

Started from first principles. is that the api we really want?.

A

I know it was implemented that way, before,, but is it the way we really want it?.

A

I'm going to ask about the function. Names., I'm going to ask about the comments.

A

I'm going to ask about the testing and there's at least one pull request. That's.

A

Stuck in my queue right, now.

A

somebody took a block of complicated code out of kubernetes communities,.

A

Moved it to kubernetes utils and I went, this is interesting.. I'm not gonna,.

A

I'm going to actually think really hard about this. and I found a massive,.

A

Huge fundamental flaw in the code and basically kind of needs to be rewritten.

A

Like the whole api is wrong., it expresses something that isn't going to work.

A

That code still exists in kubernetes communities, right?, the bugs still exist in communities, communities and now got to go back and figure out. How do we fix that original.

A

Bug before we try to move the code out, again. right., but it was only because it came up to the utils level that I even took a look at.

A

A

So the comment being, if you read a wrote, a piece of code in the first place or.

A

If you're very familiar with it,, you can come back later and you tend to assume that it still works the way you.

A

Left it. but there's hundreds and hundreds of thousands and thousands of people changing.

A

Our code base., like that's, not actually a safe assumption and it's,. It's true,.

A

Right. I have found code and changes that,.

A

That I don't understand- and I don't know where they came from. and I go run,.

A

Get blamed and I say, oh, somebody made this change. okay,.

A

Let me go backtrack to try to figure out why this change was made.

A

And most of the time it's fine and it's correct., but sometimes I track back and I go, oh,. I disagree with the way this was done.

A

Right? and there's really no answer to that..

A

It's a distributed project with lots of people who have owner's.

A

Responsibilities. and so there's no,, there's no blame here, right?.

A

I don't, blame is not a useful thing in this point,, but understanding what,.

A

What happened in? How is important.

A

And often that means like where's the test? right?.

A

Was there a test that covered this thing in the first place? right??

A

So every time you take a shortcut and you write a piece of code and you go, and the test for this would be hard. I'll, just skip it., I'm like, you're laying a time bomb for yourself.

A

So the question is,: what's my personal workflow? um,.

A

My personal workflow is a little weird and a little unfortunate. um,.

A

Basically, at this point, if I am not assigned something and github,.

A

I'm probably won't see it. now I actually do,.

A

I can actually show it if it's,, if it's interesting. um,, I probably shouldn't.

A

I use, it's my mailbox. um,.

A

I use gmail saved searches,, so sorry,.

A

Multiple inbox and I create multiple inboxes with different queries and then I.

A

Set the limit to sh to show messages to zero.

A

So what I get on my screen is a bunch of lines that have a view all button and.

A

The view all button is like a saved query., so I have my usual routine.

A

I come in in the morning,. The first button I hit is everything in my inbox that is not kubernetes related.

A

And I read that and then it's everything in my inbox. That's not.

A

There. sorry everything my inbox says directly to me: that's not a copy or a mailing.

A

List or something else. right. and I go read those and then I have github p: zero github p, one github p to.

A

Github p, zero is everything from github that is assigned to me and it.

A

Automatically gets a red label in my, in my mailbox view., but when I click that,.

A

When I see as all the prs and issues that are assigned to me, right.,.

A

And I read that pretty much every day. and so if you assign an issue to me,.

A

I will see it right now: go back to the main view and hit github p one.

A

That's all the issues that mentioned me. right?

A

or where the system has automatically asked me for review,, but it isn't assigned to me.. I don't view that every day.

A

I don't even view that every week. right.- I don't get that far- every week.

A

So this is why some of the people are pinging me on an issue, and I don't ever.

A

See it cause it's stuck in that.

A

And [inaudible] is, I think, things that are like group mentioned.

A

So like not that important,, but I get a ton of github p, ones. right.

A

And I feel bad because I'm missing interesting and useful stuff,.

A

But there's only so much a single person can do and I agree that the tooling is.

A

Sort of lacking. this is the best mechanism. I've found.

A

I need to wrap up. if anybody has other questions,, I'm happy to sit and pontificate about code review..

A

If you feel like you're struggling with code review, like you, don't feel comfortable or safe, doing code review or pushing back on.

A

People, talk to me and then we can talk through it. everybody, here,.

A

Anybody who's willing to do a code review,. You are my best friend and you are my first line of defense against the other.

A

Onslaught of brain rotted, zombies. no,, that's wrong.! That's a movie.

A

Your,, you are the defense against entropy, right? left to its own devices.

A

All code will rot. it's our job to make sure it doesn't. thank you.