This page offers basic guidelines for reviewers who comment on change sets in the Linux Foundation code-review system Gerrit, especially on new bodies of work. Please note the ideal service level agreement (SLA) is that a reviewer shall respond within 36 hours of being added as a reviewer to a change set.
When to post -2, -1, +1, +2
Reviewers indicate their opinion by posting a code-review vote as follows:
- Major defects: -2, this shall not be merged. Hopefully you will not ever need to vote this way.
- Significant concerns: -1, I would prefer this is not merged as is
- I approve: +1, Looks good to me, but someone else must approve
- Merge this: +2, Looks good to me, approved
Only committers can vote +2 and merge. Please note that committers are STRONGLY encouraged to avoid merging their own code if at all possible.
What about 0? Some people add comments without disapproving or approving. This may be appropriate when the reviewer lacks technical or domain expertise on the feature being changed.
Level 0 Review - Policy Compliance
These criteria check compliance with Linux Foundation and Acumos project policies:
- The commit message complies with the required format:
- Short first line summary, up to 50 characters
- Blank line
- Details about the change
- Blank line
- Issue-Id, Change-Id and Signed-off-by lines are present
- No binary files - no exe, jar, tgz, zip or other non-source files.
- Exception: picture files like gif, jpg and png are acceptable
- The file license.txt is present in the repository root
- All text files that support comments have a license statement at the top. For example, this generally includes all source code and RST files, but excludes data files like JSON
- No files from third parties are present, especially not files that assert licenses other than Apache 2.0 or compatible
If a change set does not meet this level, I feel a -2 vote is appropriate.
Level 1 Review - Sanity and Reusability
These criteria check if the code in the change set can be used by others:
- The file readme.md (markdown format) is present in the repository root and gives an overview of the contents
- The file doc/release-notes.rst (Real Simple Text format) is present and contains a concise statement of the change set
- The project has build and test specifications; e.g., Maven pom.xml for java and Tox for python
- The Jenkins server has a verify job, and the verify job succeeds on the change set
- Every server (i.e., non-library) project must have a suitable build specification to yield a Docker image
If a change set does not meet this level, I feel a -1 vote is appropriate.
Level 2 Review - Java Code Practices
These criteria apply to Java code changes. It's impossible to cover all cases here so this section focuses on specific Acumos concerns and some common pitfalls:
- Configuration properties should be documented appropriately and an example template should be provided
- All code should log debug and errors via the SLF4J library
- Servers should include the Logback library and logback configuration file with the recommended output format.
- JUnit tests must be included to achieve at least 50% coverage of the statements as measured by Sonar.
- Log (don't discard) exceptions. Stated differently, a catch block must never be empty.
Avoid resource leaks by appropriate use of try-finally constructs.
Level 2 Review - Python Code Practices
These criteria apply to Java code changes
- Tox tests must be included to achieve at least 50% coverage of the statements as measured by Sonar
- TBD by a true Python expert.
These criteria apply to web-site code changes
- TBD by a true front-end expert.