From 90be290bade24fd6950d6ffcd685f31bb5b0ab13 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 5 Apr 2018 18:33:45 +0100 Subject: [PATCH] docs: Add PR review guide Add a document with tips on how to review a PR. Fixes #24. Signed-off-by: James O. D. Hunt --- CONTRIBUTING.md | 2 + PR-Review-Guide.md | 136 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+) create mode 100644 PR-Review-Guide.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bb4ae9db3..653fc4fa7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -183,6 +183,8 @@ likely to be identified during review. Before your PRs are merged into the main code base, they are reviewed. We encourage anybody to review any PR and leave feedback. +See the [PR review guide](PR-Review-Guide.md) for tips on performing a careful review. + We use an "acknowledge" system for people to note if they agree or disagree with a PR. We utilize some automated systems that can spot common acknowledge patterns, which include placing any of these **at the beginning of a comment diff --git a/PR-Review-Guide.md b/PR-Review-Guide.md new file mode 100644 index 000000000..b91e58514 --- /dev/null +++ b/PR-Review-Guide.md @@ -0,0 +1,136 @@ +* [High level considerations](#high-level-considerations) + * [Security](#security) + * [Reliability](#reliability) + * [Performance](#performance) + * [Maintenance](#maintenance) + * [Usability](#usability) +* [Specifics to consider](#specifics-to-consider) + * [Basics](#basics) + * [Layout and formatting](#layout-and-formatting) + * [Design](#design) + * [Code comments](#code-comments) + * [Documentation](#documentation) + * [Logging and debugging](#logging-and-debugging) + * [Testing](#testing) + * [Environments](#environments) + * [Upgrades](#upgrades) +* [Mandatory checks](#mandatory-checks) + +--- + +This document attempts to capture some of the points to consider when +reviewing a [new code contribution](CONTRIBUTING.md) (a Pull Request (PR)) to +ensure overall quality is high. + +## High level considerations + +### Security + +- Does the change introduce a potential security risk? + - What additional checks resolve the potential security risk? + +### Reliability + +- What happens on failure? Consider **every** possible failure. For example: + - Are temporary files left on the disk? + - Are any resources (e.g. memory and open files) leaked? + +- If the code crashes, what is the overall system impact? + - Can the system be restarted and recovered? + - Is [security](#security) compromised? + +### Performance + +- Does this change negatively impact performance? +- Can any calls block? If yes, there must be a log call before the blocking call explaining what is about to be done. + +### Maintenance + +- How easy is this code to maintain: + - By an experienced developer on the project? + - By a developer with none or almost no experience of the code base or the project? + +### Usability + +- Does the change improve usability or make it worse? +- Can usability be improved further? + +## Specifics to consider + +### Basics + +- Consider *all* inputs and outputs. +- What resources are being used (e.g. memory, disk, etc.)? Are the resources released? +- Are all return values and arguments checked? +- Are there any magic numbers? + +### Layout and formatting + +- Is formatting and naming consistent? +- Are there any spelling mistakes ("typos")? +- Do all new files contain the required licence header? + +### Design + +- Can the code be simplified or refactored? + - Is the code "too clever"? Overly clever code can be fragile. Reject it unless there is a **very** good documented reason. +- Is there a better (simpler) solution to the problem? +- Is there any duplication? +- Does the code "reinvent the wheel?" Is there a standard package you can use instead? +- What assumptions does the code make? Are the assumptions valid and documented? +- Does the code make sense? + - Someone with minimal exposure to the codebase should be able to guess what the code is doing. + +### Code comments + +- Is the code well commented? +- Are all functions and function parameters documented. For `golang` programs, is their purpose obvious? +- Does this change require an update to the README(s) or architecture docs, installation docs, or limitations docs? +- Are any lines commented out? If yes, remove them. +- Are there any `FIXME` or `TODO`'s? If yes, replace them with a full URL to an issue number tracking the work to be done. + +### Documentation + +- Should the document update be applied to any *other* documents? + For example, when a PR updates an installation guide, determine whether those changes apply to one or more of the other installation guides. +- Does the PR make any of the following changes? If yes, determine if a documentation change is necessary: + - New programs or scripts are added or removed. + - New program or script options are added or removed. + - New config options are added or removed. + - New features are added or removed. + - Existing bug(s) are fixed. + +### Logging and debugging + +- Can someone debug this code from a logfile if it fails? +- Are all fatal scenarios logged? +- Are error messages and log calls useful and comprehensive? Ensure sensitive information is **not** displayed. + +### Testing + +- Is the code easy to test? +- Are there new unit tests? If not, why not? +- What other classes of testing (e.g. integration, system, stress, fuzz) can someone write to check the code? + +### Environments + +- Does the code work on all distributions? +- Does the code work on all architectures? +- What environments does your code run in? +- Which user does the code run as? If it runs as the superuser, have clear documentation to explain why this is necessary. + +### Upgrades + +- If the code logs internal state to a file or shared memory, how are upgrades handled? +For example, if a new version introduces a new state file format, does the new version consume the old state file format files? +- If upgrades cannot be handled, document this. + +## Mandatory checks + +- Provide constructive comments on the code. + - If you particularly like some aspect of the code, say so. + - If you do not like something about the code, respectfully state what you do not like. + - If you do not understand the code, say so because code should **always** be clear. + +- Ensure all automated checks pass. +- Ensure the PR contains at least one `Fixes #XXX` commit message referencing an issue that this PR fixes.