... | ... | @@ -18,7 +18,7 @@ |
|
|
|
|
|
![MR_workflow](uploads/f33a66b859e990f79bf380d6287d411c/MR_workflow.jpg)
|
|
|
|
|
|
A Merge Request can be created when the code is structuraly stable and reviewable.
|
|
|
A Merge Request can be created when the code is structurally stable and reviewable.
|
|
|
|
|
|
Regarding the CI results analysis, the following tests are unstable and may be disregarded, especially if you are working on 5G :
|
|
|
- eNB-CI-IF4p5-TDD-Band40-B210
|
... | ... | @@ -26,11 +26,6 @@ Regarding the CI results analysis, the following tests are unstable and may be d |
|
|
- eNB-CI-MONO-FDD-Band13-X2HO-B210
|
|
|
- eNB-CI-TDD-Band40-B210
|
|
|
|
|
|
The developer is primarily responsible for manual inspection an pre-filtering of the CI results.
|
|
|
If any doubt, the CI team is of course available to review the results.
|
|
|
|
|
|
Once the MR is tagged READY_TO_BE_MERGED, developers shall not push again to that branch
|
|
|
|
|
|
|
|
|
## Committee Role ##
|
|
|
|
... | ... | @@ -51,25 +46,38 @@ The Technical Committee is responsible for the following tasks: |
|
|
7. Review all the policies to be in-line with state-of-the-art development tools
|
|
|
|
|
|
|
|
|
## MR and Review Rules ##
|
|
|
## Merge Requests and Review Rules ##
|
|
|
|
|
|
A Merge-Request can be submitted as soon as the code is considered stable and reviewable.
|
|
|
|
|
|
**On test side**, the MR triggers the CI pipeline to test the proper integration into the current develop branch.
|
|
|
The developer is primarily responsible for manual inspection and pre-filtering of the CI results.
|
|
|
The CI team is of course available to review the results and re-run some tests individually to dispel any doubts.
|
|
|
|
|
|
Once the CI is passing (pre-requisite), the MR can be tagged READY_TO_BE_MERGED, developers **shall not push again to that branch**
|
|
|
|
|
|
The CI passing on the MR is a pre-requisite to start the review process.
|
|
|
**On review side**, in parallel to the testing process, TC members or peer developers are tasked to perform a human code review.
|
|
|
|
|
|
During a Merge-Request, one or several TC members are tasked to perform a human code review.
|
|
|
The objective of the review is to check things that were not automatically checked by CI, guarantee a qualitative level of the current contributions and improve the quality of the future ones.
|
|
|
The objective of the review is to:
|
|
|
- Check things that were not automatically checked by CI
|
|
|
- Guarantee a qualitative level of the contributions
|
|
|
- Improve the quality of the future 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 idea is to start the review early enough so that:
|
|
|
- The developer can still consider fixes and recommendations before his/her work is integrated
|
|
|
- The integration is not gated for too long, especially for large contributions
|
|
|
|
|
|
The review form in markdown format is public and attached to the merge request.
|
|
|
The review form is in markdown format, is public and is attached to the merge request.
|
|
|
|
|
|
The outcome of the review is in 2 parts:
|
|
|
- a 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 for the current merge or in the future
|
|
|
- A 4 level integration status : accepted / minor revisions required / major revisions required / rejected
|
|
|
The 2 first levels will allow the contribution to be approved.
|
|
|
- A recommendation status : the reviewer provides the contributor with suggestions to enhance/improve his/her work for the current merge or 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 developer is reponsible for warning 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.
|
|
|
When preparing a contribution that is larger than average, the developer is responsible for warning the OAI team, so that the review work can start as early as possible and run in parallel 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.
|
|
|
|
|
|
|
|
|
## Review form ##
|
... | ... | @@ -80,28 +88,32 @@ Form to use by copying/pasting into a comment on the Merge Request Web page: |
|
|
|
|
|
**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
|
|
|
* [ ] Indentation is not respected
|
|
|
* [ ] Insufficient in-code comments / doc
|
|
|
* [ ] 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?
|
|
|
* [ ] **IMPACT on FUNCTIONAL CODE**
|
|
|
- [ ] The code change does **NOT** respect architecture/protocol split
|
|
|
- [ ] Abnormal exits are **NOT** properly handled
|
|
|
- [ ] Coding Style Issues
|
|
|
* [ ] Added DEAD Code
|
|
|
* [ ] Improper logging
|
|
|
* [ ] Added useless debug code
|
|
|
* [ ] Duplication of an existing function
|
|
|
* [ ] Indentation is not respected
|
|
|
* [ ] Insufficient in-code comments / code readability
|
|
|
* [ ] Any other coding style issue
|
|
|
- [ ] A new tool/lib/package has been introduced (blocking, subject to negotiation)
|
|
|
* [ ] **TESTING** (The Merge Request requires additional testing)
|
|
|
- [ ] Additional testing is present in the Merge Request
|
|
|
- [ ] The contributor provides a framework for testing
|
|
|
- [ ] New testing means need to be developed
|
|
|
* [ ] **DOCUMENTATION** (The Merge Request requires additional documentation)
|
|
|
- [ ] Feature Set documentation update needed
|
|
|
* [ ] DONE
|
|
|
* [ ] NOT DONE
|
|
|
- [ ] Wiki tutorial / in-repo usage documentation needed
|
|
|
* [ ] DONE
|
|
|
* [ ] NOT DONE
|
|
|
- [ ] Added/Modified/Removed script/build/runtime option(s)
|
|
|
* [ ] Help UPDATED
|
|
|
* [ ] Help NOT UPDATED
|
|
|
|
|
|
Recommendations:
|
|
|
|
... | ... | @@ -115,28 +127,32 @@ 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
|
|
|
* [ ] Indentation is not respected
|
|
|
* [ ] Insufficient in-code comments / doc
|
|
|
* [ ] 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?
|
|
|
* [ ] **IMPACT on FUNCTIONAL CODE**
|
|
|
- [ ] The code change does **NOT** respect architecture/protocol split
|
|
|
- [ ] Abnormal exits are **NOT** properly handled
|
|
|
- [ ] Coding Style Issues
|
|
|
* [ ] Added DEAD Code
|
|
|
* [ ] Improper logging
|
|
|
* [ ] Added useless debug code
|
|
|
* [ ] Duplication of an existing function
|
|
|
* [ ] Indentation is not respected
|
|
|
* [ ] Insufficient in-code comments / code readability
|
|
|
* [ ] Any other coding style issue
|
|
|
- [ ] A new tool/lib/package has been introduced (blocking, subject to negotiation)
|
|
|
* [ ] **TESTING** (The Merge Request requires additional testing)
|
|
|
- [ ] Additional testing is present in the Merge Request
|
|
|
- [ ] The contributor provides a framework for testing
|
|
|
- [ ] New testing mean needs to be developed
|
|
|
* [ ] **DOCUMENTATION** (The Merge Request requires additional documentation)
|
|
|
- [ ] Feature Set documentation update needed
|
|
|
* [ ] DONE
|
|
|
* [ ] NOT DONE
|
|
|
- [ ] Wiki tutorial / in-repo usage documentation needed
|
|
|
* [ ] DONE
|
|
|
* [ ] NOT DONE
|
|
|
- [ ] Added/Modified/Removed script/build/runtime option(s)
|
|
|
* [ ] Help UPDATED
|
|
|
* [ ] Help NOT UPDATED
|
|
|
|
|
|
Recommendations:
|
|
|
|
... | ... | |