Tag Archives: codereview

Helpful Gerrit Queries (Gerrit 2.8 edition)

Gerrit got a very nice upgrade recently which brings in a whole new host of features that are really interesting. Here are some of the things you should know to make use of these new features. You might want to read up on the basics of gerrit searches here: Gerrit queries to avoid review overload, before getting started.

Labels

Gone are the days of -CodeReview-1, we now have a more generic mechanism called labels. Labels are a lot more powerful because they can specify both ranges as well as specific users!

For instance, to select everything without negative code reviews:

Because we now have operators, we can select for a range of values, so any negative (-1, -2, or any high negative value should it get implemented in the future) matches. Also negation is done with the 'NOT' keyword, and notable that CodeReview becomes label:Code-Review in the new system.

Labels exist for all three columns. Verified is what CI bots vote in, and Workflow is a combination of the Work in Progress (Workflow=-1) and Approved (Workflow=1) states that we used to have.

Labels with Users

Labels get really power when you start adding users to them. Now that we have a ton of CI bots voting, with regular issues in their systems, you might want to filter out by changes that Jenkins currently has a positive vote on.

This means that changes which do not yet have a Jenkins +1 or +2 won't be shown in your list. Hiding patches which are currently blocked by Jenkins or it hasn't reported on yet. If you want to see not yet voted changes, you could change that to >=0.

Labels with Self

This is where we get really fun. There is a special user, self, which means your logged in id.

This is a list of all changes that 'you have not yet commented on', that don't have negative code reviews, and that Jenkins has passing results. That means this query becomes a todo list, because as you comment on changes, positive, negative, or otherwise, they drop out of this query.

If you also drop all the work in progress patches:

then I consider this a basic "Inbox zero" review query. You can apply this to specific projects with "project:openstack/nova", for instance. Out of this basic chunk I've built a bunch of local links to work through reviews.

File Matching

With this version of gerrit we get a thing called secondary indexes, which means we get some more interesting searching capabilities. which basically means we also have a search engine for certain other types of queries. This includes matching changes against files.

is a query that looks at all the outstanding changes in OpenStack that change a database migration. It's currently showing glance, heat, nova, neutron, trove, and storyboard changes.

Very helpful if as a reviewer you want to keep an eye on a cross section of changes regardless of project.

Learning more

There are also plenty of other new parts of this query language. You can learn all the details in the gerrit documentation.

We're also going to work at making some of these "inbox zero" queries available in the gerrit review system as a custom dashboard, making it easy to use it on any project in the system without building local bookmarks to queries.

Happy reviewing!

 

Why you should be reviewing more OpenStack code

“Read, read, read. Read everything—trash, classics, good and bad, and see how they do it. Just like a carpenter who works as an apprentice and studies the master. Read! You’ll absorb it.”

– William Faulkner

Icehouse 3 is upon us, and as someone that is on a bunch of core review teams, it means a steady drum beat of everyone asking how do they get core reviewers to review their code. My standard response has been "make sure you are also reviewing code".

Why?

Understanding implicit style

While most projects use the hacking program to check for trivial style issues (wrong formatting), there are a lot of other parts of style that exist inside a project. What's a good function look like? When is a project handling exceptions vs. doing checks up front. What does spacing inside functions look like. What "feels" like Nova code, and what feels foreign and odd.

This is like when you are invited to a party at someone's house for the first time. You walk in the door, and the first thing you do is look to the host, and the guests, and figure out if people are wearing shoes in the house or not. And follow suit if there looks like there is a pattern. It's about being polite and adapting to the local environment.

Because unless you read other people's code, you'll never understand these things. There are lots of patches I look at briefly, realize that they are in some whacky style that's so foreign to the code at hand, that I don't have the energy to figure out what the author means, and move on.

Taking load off review teams

As a core reviewer, I currently have about 800 patches right now that I could +2 or -2. Given the rate of code coming in, that might as well be infinite. And it grows all the time.

By reviewing code, even when you don't have approval authority, you'll be helping the review teams weed out patches which aren't in any way ready to move forward. That's a huge time savings, and one that I appreciate.

Even if it's something as simple as making sure author's provide good commit messages, that's huge. Because I'll completely skip over reviews with commit messages that I can't understand. That's your opportunity to sell me on why I should spend the next 30 minutes looking at your code. A good commit message, being really clear about what problem this code hits, and what this solution is, and why this implementation is the right approach, will make me dive in.

A terrible or unclear commit message will probably just make me ignore the code, because if the author didn't care enough to explain that to me in the commit message, there are probably lots of issues in the code itself. Even if you and I had a conversation about this code last week, don't assume I remember all of that. That was probably 50 code reviews ago for me, which means the context of that conversation has long since flushed from my brain.

If you review a bunch of code, you'll understand how these things impact your ability to review code, and will naturally adapt how you write commits (including the message) to make life of a reviewer easier.

Seeing the bigger picture

People tend to start contributing in just one corner of OpenStack, but OpenStack is a big, interconnected project. What you do in one corner can effect the rest of the project. If you aren't reviewing code and changes happening at other layers of the project, it's really hard to know how your piece fits into the larger picture.

Changes can look fine in the small, but have a negative impact on the wider project. If you are proactive in reviewing code more broadly you can see some of that coming, and won't be surprised when a core reviewer -2s you because you were going in a different direction than the rest of the project.

Becoming a better programmer

When I started on the OpenStack project 2 years ago I hadn't done python in a real way for years. My python was very rusty. But one of the first things I did was start reviewing a bunch of code, especially by some of the top people in the project.

There are some really smart and really skilled people in the OpenStack project. There are people that have been part of the python community for 15+ years. People that live and breath in python. Just reading their code makes you realize some of what can be done with the language, and what the "pythonic" way of doing things is. Nothing is better training for becoming a better python developer than learning from these folks.

Some times you'll find a real issue, because no one is perfect. Some times you'll find something you don't understand, and can leave a comment as a question, which you'll probably get an answer to. But all of it will be learning. And you will become a better developer.

It does make a difference

I'll be 100% honest, with 800+ reviews I should be looking at, I play favorites. People that I see contributing a lot on the review side (not just volume, but real quality reviews that save me time) are people who's code I want to review, because they are contributing to the whole of the project, not just their little corner.

So that's why you should review more code in OpenStack. It will really contribute to the project, make you a better developer, and through all this you'll find your code is naturally aligning better with OpenStack and gets reviewed more often. Realize this is not an overnight fix, but a long term strategy for aligning with the community and becoming part of it.