The merge request links on this page refer to the old (now read-only) athenaprivate1 repository. They are kept as illustrative examples.
This set of examples is somewhat more involved and are worth reading directly in the merge request interface to get an understanding of what is expected as part of a good code review. For non-trivial changes a shifter would first be expected to follow the Shifter Checklist to assess the correctness of any changes.
The Shifter Checklist will be adapted as experience is gathered with real use cases. It is not intended to be a prescriptive list and the shifter is encouraged to use their knowledge and initiative as part of the code review process. A discussion on the nature of the changes should be encouraged between developers, shifters and experts at the earliest opportunity in the review.
!435 - Code Discussion
This example shows a threaded discussion between reviewer and developer on a particular change within the code (Python in this case). This is generated by selecting the code line of interest in the changes panel:
After a comment is made this then generates the format seen above in the discussion panel and is as a good method to contain discussions surrounding specific changes. Once a discussion between reviewer, developer and experts has finished and a decision has been made the discussion can be marked as resolved.
!631 - Multiple Discussions
This example has multiple discussions as part of the review. For clarity each discussion thread can be compressed or expanded by selecting Toggle Discussion.
Other detailed examples
- !894 - This request has elements of design discussion and good code review (and GitLab problems!)
Process Issues
The issues below highlight some of the current issues with the build process and developer misunderstanding whilst the review process matures. You should be mindful of these kinds of issues whilst on shift in addition to the normal code review practices.
- !959 - Perfectly fine changes fail testing due to issues with the build system (test timeouts in this case)
- !427 - A trivial change was approved but a bug was introduced
- !1007 - Developer is trying to merge the same source branch into two different target branches and confusing the CI system