Our program follows the usual OpenStack review process, albeit with some important additions (see below). See also: Your first review.
The PTL, with the support of the core reviewers, is ultimately responsible for holding contributors accountable for creating a positive, constructive, and productive culture. Inappropriate behavior will not be tolerated. (Why this is important?)
Do This:
Don’t Do This:
When possible, enlist the help of a professional technical writer to help review each doc patch. All reviewers should familiarize themselves with OpenStack Documentation Contributor Guide. When reviewing user guide patches, please run them through Maven and proof the resulting docs before giving your +1 or +2.
When reviewing code patches, use your best judgment and seek to provide constructive feedback to the author. Compliment them on things they have done well, and highlight possible improvements. Also, dedicate as much time as necessary in order to provide a careful analysis of the code. Don’t assume that someone else will catch any issues you yourself miss; in other words, pretend you are the only person reviewing a given patch. Remember, “given enough eyeballs, all bugs are shallow” ceases to be true the moment individual reviewers become complacent.
Some things to check when reviewing code:
We encourage the use of prefixes to clarify the tone and intent of your review comments. This is one way we try to mitigate misunderstandings that can lead to bad designs, bad code, and bad blood.
Prefix | What the reviewer is saying | Blocker? |
---|---|---|
KUDO | You did a nice job here, and I wanted to point that out. Keep up the good work! | No |
TEST | I think you are missing a test for this feature, code branch, specific data input, etc. | Yes |
BUG | I don’t think this code does what it was intended to do, or I think there is a general design flaw here that we need to discuss. | Yes |
SEC | This is a serious security vulnerability and we better address it before merging the code. | Yes |
PERF | I have a concern that this won’t be fast enough or won’t scale. Let’s discuss the issue and benchmark alternatives. | Yes |
DSQ | I think there is something critical here that we need to discuss this in IRC or on the mailing list before moving forward. | Yes |
STYLE | This doesn’t seem to be consistent with other code and with HACKING.rst | Yes |
Q | I don’t understand something. Can you clarify? | Yes |
DRY | This could be modified to reduce duplication of code, data, etc. See also: Wikipedia: Don’t repeat yourself | Maybe |
YAGNI | This feature or flexibility probably isn’t needed, or isn’t worth the added complexity; if it is, we can always add the feature later. See also: Wikipedia: You aren’t gonna need it | Maybe |
NIT | This is a nitpick that I can live with if we want to merge without addressing it. | No |
IMO | I’m chiming in with my opinion in response to someone else’s comment, or I just wanted to share an observation. Please take what I say with a grain of salt. | No |
FYI | I just wanted to share some useful information. | No |