The simple way intended to find mistakes overlooked in the initial development phase.
The goal is to improve overall quality of the code .
Practices vary from pair programming, informal walkthroughs, and formal inspection.
ONAP uses Gerrit for Code Review
What to review?
- Comply to Coding Guidelines: see Java code style
- Comply to Intellectual Property (Copyright and license headers)
- Comply with Commit Process
- Ensure there is no Chinese Character in comments
Code reviews are expected to take place within the next 36 business hours after the code has been submitted within Gerrit.
ONAP is an Open Source project and thus anyone who is interested can provide code review.
Best candidate to ensure the code is reviewed are the “Committers” listed in project resources.
Authors are not expected to perform Code Review on their own code. This rule is especially true for integration work.
See also Committer Best Practices
7 Comments
Gildas Lanilis
Practice approved by TSC 2017-07-13 (topic #10). PDF Summary of materials presented.
Dan Timoney
2 comments on the timeframe:
1) I think the agreement is 36 business hours, not 24.
2) That timeframe should only apply to changes that (a) are in Verified state (meaning the automated test compile passed) and (b) have no merge conflicts. If either of those conditions exist, the change should not be merged until the issue is cleared and the change should remain open so the develop can clear the issue.
Pamela Dragosh
I agree with Dan. We would like to have some flexibility in staging code review submissions. We don't want to lose some valuable work from someone who is going on vacation and gets work done before others. Giving time to another person/team get some other work submitted and merged before going back and fixing what the first person had submitted. We don't want to have to wait for them to come back from vacation, or have someone else be required to duplicate their work (and then they lose credit for doing it). It would be great if we can allow merge conflicts to exist, so we can better funnel changes happening. This can be documented via "reply" messages in the code review. 36 business hours seems like a long time, but occasionally this isn't possible.
Gildas Lanilis
Instead of making exception for the scenario you have described, I would rather like to enforce the "Continuous Integration" development best practices. Key practices such as "Developer is responsible for fixing the build" or "Fix it immediately or revert the change" should be in the DMA of a developer. A merge conflict must be fixed by the developer before he leaves the office. Before a developer leaves for vacation, he should ensure his code has been integrated properly by committing frequently and not a lost minutes (at 4:55 pm on a Friday).
Gildas Lanilis
Yes, the agreement was 36 business hours. Good catch, I updated the wiki accordingly.
Adam Krysiak
I walked through few recent pull requests doing review and I see things that are done and they shouldn't be to keep quality of code on as high level as possible.
I thing every one should follow first 3 points. Rest is rather recommendation how to keep code readable and easy to maintain. Of course I'm aware of fact that we are not able to change everything instantly. I just want to improve quality of new code and encourage everyone to do code review even if you don't know component. Let's remember that it's an open source project and quality is one of most important factor when someone decides to join or use it.
Jakub Zieba
Adam Krysiak First and last three points should be checked in https://sonar.onap.org.