The Asterisk project is dedicated to ensuring that the changes made to it are of the best possible quality. Code reviews are one way the project helps to ensure that the changes made are well understood and do not have adverse effects. As an open source project, it is up to all of us to ensure that the changes made to Asterisk improve the project for the good of everyone.
Code reviews are typically done through Review Board, although code review can also take place on issues in the Asterisk issue tracker. Instructions on using Review Board can be found at Review Board Usage. In general, we encourage all patch contributors to post their patches to Review Board, as this helps to ensure that all changes to Asterisk have been reviewed by multiple developers.
- The patch author posts the patch to Review Board. The following should be filled in on the review:
- Summary: describe in brief what the patch does. If possible, it is useful to prefix the summary with the affected modules in Asterisk, e.g., chan_sip, app_queue, etc.
- Bugs: provide the issue in JIRA that the patch resolves.
- Groups: always specify the "asterisk-dev" group. This will cause the review to automatically generate an e-mail to the asterisk-dev list.
- People: often, there are subject matter experts who would be helpful in reviewing your issue. If you know of particular developers that you would like to review your patch, provide their username here. If a patch affects a module maintained by a community developer, you should provide their name here.
- Other developers will review the patch. See the Code Review Checklist for common items that developers will look for.
- If there are findings, the patch author should resolve them. Resolved findings should be closed out on the review.
- When all findings are resolved, the patch author should re-submit the patch to the same review. See Review Board Usage for instructions on updating a review.
- If a developer provides a "Ship It" for a review, that implies that the developer feels that the patch is ready for inclusion.
- If a developer comments on a review that has a "Ship It", those findings should be resolved before the patch is included.
- Patches with a "Ship It" should, when possible, be given 24 hours before being committed. This allows developers in other time zones to comment on a shipped patch.
- Once a patch has a "Ship It" and all findings are closed, the patch can be committed by a developer with commit access.
Watching Code Reviews
You can watch all code reviews that occur in the Asterisk project by subscribing to the asterisk-dev mailing list. If you are the author of a patch or you have been listed in the People field, you will be automatically e-mailed whenever a change occurs in a code review.
Unresolved Code Reviews
If a review has unresolved findings for a long period of time - 3 months from the issuing of the findings - your review may be closed out. This is to try and help peer reviewers find active reviews and not be distracted by code reviews that are no longer being actively supported by their author. If you find that your review is closed, you may always re-open it yourself, or ask for it to be re-opened in #asterisk-dev.
A Final Note on Code Reviews
Often, as developers, we take a personal pride in our work. This is a Good Thing: we should be proud of the work that we do. Submitting your work for peer review will subject your work to criticism. This is not a bad thing.
The Asterisk project is bigger than any one developer. Ensuring quality for the project is the goal for everyone who participates in it, and sometimes that may result in findings against your code. It is not personal: the developers who participate in the Asterisk project are passionate about the success of the project and want the very best to go into it.
When participating in a code review, please remember the Asterisk Community Code of Conduct, and remember that we're all trying to make Asterisk the best possible open source communications engine.