|
|
<table style="border-collapse: collapse; border: none;">
|
|
|
<tr style="border-collapse: collapse; border: none;">
|
|
|
<td style="border-collapse: collapse; border: none;">
|
|
|
<a href="http://www.openairinterface.org/">
|
|
|
<img src="../images/oai_final_logo.png" alt="" border=3 height=50 width=150>
|
|
|
</img>
|
|
|
</a>
|
|
|
</td>
|
|
|
<td style="border-collapse: collapse; border: none; vertical-align: center;">
|
|
|
<b><font size = "5">OAI Software Alliance Technical Review Guidelines</font></b>
|
|
|
</td>
|
|
|
</tr>
|
|
|
</table>
|
|
|
|
|
|
## Table of Contents ##
|
|
|
|
|
|
|
|
|
|
|
|
The Technical Commitee is responsible for the following tasks:
|
|
|
|
|
|
1. Review ticket upon creation to ensure [Ticket Policy Guidelines](policy/ticket-policy) are followed
|
|
|
2. Add a new project card in the “In Progress” projects in OAI Weekly Developers Meeting to track progress of Issue opened in the Ticket
|
|
|
3. In OAI Weekly Developers Meeting, track progress of all open issues and manage lifetime of [branches ](policy/branch-policy)
|
|
|
4. In case of a [Merge-Request(MR)/Pull-Request(PR)](policy/merge-request-policy), monitor CI outcome of the MR/PR
|
|
|
- Coordinate with contributor/developer to resolve conflicts in case of failure
|
|
|
5. In case of successful CI result of a MR/PR,
|
|
|
- one or several TC members are tasked to perform a "human" code review to ensure that policies / guidelines are followed:
|
|
|
* Especially the ones that cannot be reviewed/checked by an automated script.
|
|
|
* TC review topics can be found at [OAI TC Code Review Guidelines](guidelines/tc-code-review-guidelines).
|
|
|
- Accept MR/PR into the latest `develop` branch state.
|
|
|
- Coordinate with contributor/developer in case of conflicts.
|
|
|
6. Close tickets and delete branches upon task completion.
|
|
|
7. Review all the policies to be in-line with state-of-the-art development tools
|
|
|
|
|
|
|
|
|
The CI passing on the MR is a pre-requisite to start the review process.
|
|
|
|
|
|
During a Merge-Request, one or several TC members are tasked to perform a human code review.
|
|
|
This review objective is to check things that were not automatically checked by CI and guarantee a qualitative level of the contributions.
|
|
|
|
|
|
In principle, the review occurs before the final merge to develop branch, although a preliminary merge into the integration branch is possible while the review is on going, in order to detect potential issues and parallelize integration work.
|
|
|
|
|
|
The review form in markdown format is public and attached to the merge request.
|
|
|
|
|
|
The outcome of the review is in 2 parts:
|
|
|
- an 4 level integration status : accepted / minor revisions required / major revisions required / rejected
|
|
|
- a recommendation status : the reviewer provides the contributor with suggestions to enhance/improve his/her work in the future
|
|
|
|
|
|
We highly recommend to submit small granular contributions rather than large ones.
|
|
|
The goal is to make the review and integration loop as short as possible.
|
|
|
|
|
|
When preparing a contribution that is larger than average, the developper is reponsible to warn the OAI team, so that the review work can start as early as possible and run in parallele to the contribution finalization. Failing to do so, there is a risk that the integration of his/her work remains on hold/under review for a longer time.
|
|
|
|
|
|
|
|
|
|
|
|
Form to use by copying/pasting into a comment on the Merge Request Web page:
|
|
|
|
|
|
```markdown
|
|
|
|
|
|
**Code Review by : AuthorName**
|
|
|
|
|
|
* [ ] Impact on functional code
|
|
|
- [ ] Does code change respect architecture/protocol split?
|
|
|
- [ ] Are abnormal exits properly handled?
|
|
|
- [ ] Coding Style Issues (all SHALL be unselected):
|
|
|
* [ ] Added DEAD Code
|
|
|
* [ ] Improper logging
|
|
|
* [ ] Added useless debug code
|
|
|
* [ ] Duplication of an existing function
|
|
|
* [ ] Any other coding style issue
|
|
|
- [ ] Has a new tool been introduced?
|
|
|
* [ ] if yes, can an existing tool be improved?
|
|
|
- [ ] if no, do we accept this new tool?
|
|
|
* [ ] Testing --> Merge Request requires additional testing
|
|
|
- [ ] Additional testing already present in the Merge Request
|
|
|
* [ ] if no, has contributor provided framework to do so
|
|
|
* [ ] Documentation --> Merge Request requires additional documentation
|
|
|
- [ ] Feature Set documentation updated?
|
|
|
- [ ] Wiki tutorial / in-repo usage documentation?
|
|
|
- [ ] Added/Modified/Removed script/build/runtime option(s)
|
|
|
* [ ] Help modified?
|
|
|
|
|
|
Additional Comments:
|
|
|
|
|
|
-
|
|
|
-
|
|
|
-
|
|
|
```
|
|
|
|
|
|
Example after pasting into a comment:
|
|
|
|
|
|
**Code Review by : Raphael Defosseux**
|
|
|
|
|
|
* [x] Impact on functional code
|
|
|
- [x] Does code change respect architecture/protocol split?
|
|
|
- [x] Are abnormal exits properly handled?
|
|
|
- [ ] Coding Style Issues (all SHALL be unselected):
|
|
|
* [ ] Added DEAD Code
|
|
|
* [ ] Improper logging
|
|
|
* [ ] Added useless debug code
|
|
|
* [ ] Duplication of an existing function
|
|
|
* [ ] Any other coding style issue
|
|
|
- [ ] Has a new tool been introduced?
|
|
|
* [ ] if yes, can an existing tool be improved?
|
|
|
- [ ] if no, do we accept this new tool?
|
|
|
* [x] Testing --> Merge Request requires additional testing
|
|
|
- [x] Additional testing already present in the Merge Request
|
|
|
* [ ] if no, has contributor provided framework to do so
|
|
|
* [x] Documentation --> Merge Request requires additional documentation
|
|
|
- [x] Feature Set documentation updated?
|
|
|
- [ ] Wiki tutorial / in-repo usage documentation?
|
|
|
- [ ] Added/Modified/Removed script/build/runtime option(s)
|
|
|
* [ ] Help modified?
|
|
|
|
|
|
Additional Comments:
|
|
|
|
|
|
- None
|
|
|
|