... | @@ -18,7 +18,7 @@ |
... | @@ -18,7 +18,7 @@ |
|
|
|
|
|
![MR_workflow](uploads/f33a66b859e990f79bf380d6287d411c/MR_workflow.jpg)
|
|
![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 :
|
|
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
|
|
- eNB-CI-IF4p5-TDD-Band40-B210
|
... | @@ -26,11 +26,6 @@ Regarding the CI results analysis, the following tests are unstable and may be d |
... | @@ -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-MONO-FDD-Band13-X2HO-B210
|
|
- eNB-CI-TDD-Band40-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 ##
|
|
## Committee Role ##
|
|
|
|
|
... | @@ -51,25 +46,38 @@ The Technical Committee is responsible for the following tasks: |
... | @@ -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
|
|
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:
|
|
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.
|
|
- 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:
|
|
The outcome of the review is in 2 parts:
|
|
- a 4 level integration status : accepted / minor revisions required / major revisions required / rejected
|
|
- 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
|
|
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.
|
|
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.
|
|
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 ##
|
|
## Review form ##
|
... | @@ -80,28 +88,32 @@ Form to use by copying/pasting into a comment on the Merge Request Web page: |
... | @@ -80,28 +88,32 @@ Form to use by copying/pasting into a comment on the Merge Request Web page: |
|
|
|
|
|
**Code Review by : AuthorName**
|
|
**Code Review by : AuthorName**
|
|
|
|
|
|
* [ ] Impact on functional code
|
|
* [ ] **IMPACT on FUNCTIONAL CODE**
|
|
- [ ] Does code change respect architecture/protocol split?
|
|
- [ ] The code change does **NOT** respect architecture/protocol split
|
|
- [ ] Are abnormal exits properly handled?
|
|
- [ ] Abnormal exits are **NOT** properly handled
|
|
- [ ] Coding Style Issues (all SHALL be unselected):
|
|
- [ ] Coding Style Issues
|
|
* [ ] Added DEAD Code
|
|
* [ ] Added DEAD Code
|
|
* [ ] Improper logging
|
|
* [ ] Improper logging
|
|
* [ ] Added useless debug code
|
|
* [ ] Added useless debug code
|
|
* [ ] Duplication of an existing function
|
|
* [ ] Duplication of an existing function
|
|
* [ ] Indentation is not respected
|
|
* [ ] Indentation is not respected
|
|
* [ ] Insufficient in-code comments / doc
|
|
* [ ] Insufficient in-code comments / code readability
|
|
* [ ] Any other coding style issue
|
|
* [ ] Any other coding style issue
|
|
- [ ] Has a new tool been introduced?
|
|
- [ ] A new tool/lib/package has been introduced (blocking, subject to negotiation)
|
|
* [ ] if yes, can an existing tool be improved?
|
|
* [ ] **TESTING** (The Merge Request requires additional testing)
|
|
- [ ] if no, do we accept this new tool?
|
|
- [ ] Additional testing is present in the Merge Request
|
|
* [ ] Testing --> Merge Request requires additional testing
|
|
- [ ] The contributor provides a framework for testing
|
|
- [ ] Additional testing already present in the Merge Request
|
|
- [ ] New testing means need to be developed
|
|
* [ ] if no, has contributor provided framework to do so
|
|
* [ ] **DOCUMENTATION** (The Merge Request requires additional documentation)
|
|
* [ ] Documentation --> Merge Request requires additional documentation
|
|
- [ ] Feature Set documentation update needed
|
|
- [ ] Feature Set documentation updated?
|
|
* [ ] DONE
|
|
- [ ] Wiki tutorial / in-repo usage documentation?
|
|
* [ ] NOT DONE
|
|
- [ ] Added/Modified/Removed script/build/runtime option(s)
|
|
- [ ] Wiki tutorial / in-repo usage documentation needed
|
|
* [ ] Help modified?
|
|
* [ ] DONE
|
|
|
|
* [ ] NOT DONE
|
|
|
|
- [ ] Added/Modified/Removed script/build/runtime option(s)
|
|
|
|
* [ ] Help UPDATED
|
|
|
|
* [ ] Help NOT UPDATED
|
|
|
|
|
|
Recommendations:
|
|
Recommendations:
|
|
|
|
|
... | @@ -115,28 +127,32 @@ Example after pasting into a comment: |
... | @@ -115,28 +127,32 @@ Example after pasting into a comment: |
|
|
|
|
|
**Code Review by : Raphael Defosseux**
|
|
**Code Review by : Raphael Defosseux**
|
|
|
|
|
|
* [x] Impact on functional code
|
|
* [ ] **IMPACT on FUNCTIONAL CODE**
|
|
- [x] Does code change respect architecture/protocol split?
|
|
- [ ] The code change does **NOT** respect architecture/protocol split
|
|
- [x] Are abnormal exits properly handled?
|
|
- [ ] Abnormal exits are **NOT** properly handled
|
|
- [ ] Coding Style Issues (all SHALL be unselected):
|
|
- [ ] Coding Style Issues
|
|
* [ ] Added DEAD Code
|
|
* [ ] Added DEAD Code
|
|
* [ ] Improper logging
|
|
* [ ] Improper logging
|
|
* [ ] Added useless debug code
|
|
* [ ] Added useless debug code
|
|
* [ ] Duplication of an existing function
|
|
* [ ] Duplication of an existing function
|
|
* [ ] Indentation is not respected
|
|
* [ ] Indentation is not respected
|
|
* [ ] Insufficient in-code comments / doc
|
|
* [ ] Insufficient in-code comments / code readability
|
|
* [ ] Any other coding style issue
|
|
* [ ] Any other coding style issue
|
|
- [ ] Has a new tool been introduced?
|
|
- [ ] A new tool/lib/package has been introduced (blocking, subject to negotiation)
|
|
* [ ] if yes, can an existing tool be improved?
|
|
* [ ] **TESTING** (The Merge Request requires additional testing)
|
|
- [ ] if no, do we accept this new tool?
|
|
- [ ] Additional testing is present in the Merge Request
|
|
* [x] Testing --> Merge Request requires additional testing
|
|
- [ ] The contributor provides a framework for testing
|
|
- [x] Additional testing already present in the Merge Request
|
|
- [ ] New testing mean needs to be developed
|
|
* [ ] if no, has contributor provided framework to do so
|
|
* [ ] **DOCUMENTATION** (The Merge Request requires additional documentation)
|
|
* [x] Documentation --> Merge Request requires additional documentation
|
|
- [ ] Feature Set documentation update needed
|
|
- [x] Feature Set documentation updated?
|
|
* [ ] DONE
|
|
- [ ] Wiki tutorial / in-repo usage documentation?
|
|
* [ ] NOT DONE
|
|
- [ ] Added/Modified/Removed script/build/runtime option(s)
|
|
- [ ] Wiki tutorial / in-repo usage documentation needed
|
|
* [ ] Help modified?
|
|
* [ ] DONE
|
|
|
|
* [ ] NOT DONE
|
|
|
|
- [ ] Added/Modified/Removed script/build/runtime option(s)
|
|
|
|
* [ ] Help UPDATED
|
|
|
|
* [ ] Help NOT UPDATED
|
|
|
|
|
|
Recommendations:
|
|
Recommendations:
|
|
|
|
|
... | | ... | |