Fuel/Code Review Rules
Why do we need code review?
Code review is essential to sustainable software development process. It serves the following goals:
- Confirm that proposed code change is suitable for its purpose: it solves the stated problem and follows the agreed architecture.
- Analyze external impact of the code change: it doesn't introduce new issues, doesn't reduce stability and performance of the whole solution.
- Enforce code quality standards: the proposed change is well structured, well documented, and is consistent with the coding style of the surrounding code.
- Spread knowledge about architecture and implementation details, keep other developers up to date with current state of the code base.
Submitting your code for review
- Make sure your commit message describes the proposed code change adequately and is in line with commit message guidelines.
- Make sure your code improves overall quality of the surrounding code, avoid quick and dirty workarounds, follow code quality standards.
- Remember that it is your responsibility to get other developers to review and and approve your commit. Be proactive and responsive, use IRC and mailing list to draw attention of core developers to your review requests, and try to address all review comments promptly.
Reviewing other people's code
- Be nice. Frame your comments as helpful suggestions.
- Before the review, find out why the code is written: what is the exact problem/feature this code is written for?
- Understand the use cases for the code, data flow.
- Example: review request for node agent. Use cases: agent is ran from rc.local at boot time, it checks nailgun url from cmdline and does POST request to API, posting the data obtained from ohai.
- Are these use cases addressed in the code?
- Example: check that agent will be run at the boot time, that it collects data and does POST request
- Make sure the code does not violate the documented architecture. Changes to architecture should be discussed in blueprints and on the mailing list.
- Example: Deployment orchestrator lacks some data about node, which could be easily obtained from Nailgun API, and junior developer uses GET request to Nailgun API to get the data.
- Reviewer comment could be: "Orchestrator obtains all the data via AMQP, and it should not get any data from Nailgun API. Orchestrator must work with no use of Nailgun, so my suggestion is to extend Nailgun code so required data is sent over AMQP, along with other data."
- Take a piece of paper and write down negative scenarios. Does the code provided handles all of them? Is it written in the way to handle even possible errors, not mandatory happening right now?
- Example: agent - What if ohai doesn't provide some data? What if POST request fails due to temporary network connectivity issues? What if object is already created in backend, and POST returns error? Could it be that agent starts before network connectivity is established? Any case that url is not in cmdline?
- Make sure unit tests are present and cover:
- use cases defined in the blueprint or bug
- negative scenarios
- If there are other requirements, such as performance/scalability/threading-safety, check for specific tests too.
- Check if integration test is present or planned when required. Good reason to ask for integration test if new code affects RPC call to deployment orchestrator: new data provided or the new method call is created.
Policy for merging to fuel-library repository is now more restrictive. Core reviewers can approve commits for merge when following criteria are met:
- There's at least one +1 from an SME (subject matter expert)
- There's at least one +1 from another reviewer
- There's a +2 from another core reviewer if patch is complex, suspicious, or introduces changes to reference architecture
Commit message guidelines
A short checklist for git commit messages:
- First line of the commit message is a short (50 characters) summary of the change and is separated from the rest of the message by a blank line.
- Commit message includes one or more references to relevant bugs and blueprints.
- Body of the commit message includes:
- justification (e.g. explanation of the root cause of the referenced bug)
- scope (what parts of the referenced bug or blueprint are and are not addressed by this change)
- impact of the change on architecture, security, reliability, and performance where applicable.
For more details, see OpenStack git commit good practices. Also see Writing good commit messages from Erlant/OTP wiki for a good short summary of commit message rules. Examples of good commit messages that should be followed:
Code quality standards
No code duplication. Follow the DRY principle.
Correctness, simplicity and clarity come first. Unnecessary cleverness should be avoided. If cleverness is the must, make sure it was encapsulated, surrounded by helpful comments and good written tests are provided.
Use meaningful identifiers, do not make up abbreviations (it's ok to use well known acronyms like TTL). Use nouns for classes and verbs for methods. Make sure your identifiers are easy to tell apart.
Unused code must be removed. If a code block is not called from anywhere, do not leave it around and do not comment it out. Otherwise it will cause confusion and someone will eventually use it, and by then it will probably be obsolete and broken.
Code must be modular. If you have too many lines or too much indentation in a single method, too many methods in a class, too many levels of inheritance hierarchy, code becomes unreadable. Find a way to break it up.
Code must be loosely coupled. If there are too many dependencies between internal aspects of different modules, changing one module will keep breaking the other. Find a way to reduce interaction between modules to a simple well defined interface.
Check that any errors in the code will appear in the log file with traceback.
try: # some code that fails except: pass
try: # some code that fails except Exception as exc: # some code that processes exc logger.error(traceback.format_exc())