The only way that code changes can propagate into the main ATLAS software repository is through a merge request, which needs to be reviewed. So, reviewing code is a key feature of the new development process. This gives you the unique chance of reviewing code and help us to maintain high quality software standards. The GitLab page for a merge request provides you with plenty of information:
Below you find some guidelines what to look out for, but it all comes down to applying good coding practices with some common sense. Well, and changes, of course, should be like Oh, debugging would be a pleasure if all code (and comments and commit messages) were written like this.
For the tutorial we have prepared a merge request for every participant to review. To find it go to the
atlas/athenamerge request page. Enter your name in the search box and you should find a merge request for you. Please try to review it following the checklist below:
Quick checklist
- Check the merge request description:
- short but concise title (possibly including a JIRA reference),
- succinct description explaining why the changes were necessary, mentioning relevant consequences (e.g. changed behaviour, updated pre- or post-conditions),
- references to related JIRA issues, especially JIRA issues which are solved
by this merge request should be mentioned with a closing pattern
(e.g.
Closes ATLASRECT-1234).
- Pay attention to comments from the CI system (
atlasbot):- Is the list of affected packages sensible for the described changes?
- Does the CI system report any problems?
- Check result of automatic build and unit tests!
- Investigate changes introduced by this merge request:
- Is the list of changed files consistent with the purpose of the merge request?
- Is the code well-documented?
- Do you understand what it is doing?
- Are there obvious things which could be improved/simplified?
- Does it follow the ATLAS Coding Guidelines?
- Use the per-line-comment functionality extensively! If something is not clear today, it won’t be in 1 year.
Approve changes using labels
It is foreseen that every merge request needs to be approved by software review shifters. During working days there will be two 1st-level and two 2nd-level shifters reviewing all open merge requests.
The approval of merge requests by software review shifters is a key ingredient in the new software development workflow. Nevertheless, situations may occur where a change needs to go into the central repository as quickly as possible (or during the weekend). Therefore, release coordinators are always able to accept a merge request regardless of the outcome or status of the review process. This power should be used responsibly. In all cases a short comment explaining the urgency for short-circuiting the review process must be added.
The status of the review process is described by the following GitLab labels which are visible on the GitLab merge request page:
:
The review process is still on-going.
:
The merge request is ready to be accepted
:
The changes in their current form are rejected.
In addition, there are three more labels indicating the stage of the review process:
:
The merge request is awaiting feedback from a first-level software review shifter.
:
The merge request is awaiting feedback from a second-level software review shifter.
:
Feedback from a software domain expert is required for this merge request.
Upon creation every merge request is flagged with the review-pending and 1st-level-pending labels. A first-level shifter should then review this merge request. The outcome of the review should be summarized in a comment and the 1st-level-pending label must be removed. If there are no open/controversial points and the first-level shifter feels confident, the merge request can be approved or rejected by replacing review-pending with the appropriate label. If no decision can be taken yet, the 2nd-level-pending flag must be added.
Now the second-level software review shifter should investigate the merge
request, taking into account the discussion between the first-level shifter and
the developer. A summary of his/her review should be included as a comment and
the 2n-level-pending label must be removed. If further feedback from a
software domain expert is required, the expert-pending flags must be added and
the corresponding people should be asked for feedback using the @username
functionality of GitLab comments. Once all open issues/questions are settled,
the merge request can be approved/rejected by replacing review-pending with
the appropriate label. Any *-pending labels must be removed.
This review workflow is summarized in the picture below.
