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/athena
merge 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:
Closes ATLASRECT-1234
).atlasbot
):
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:
In addition, there are three more labels indicating the stage of the review process:
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.