Reviewing issues

We use GitLab issues to track work items. If you are addressing the issue you should add the workflowReview Required label when you want the issue to be reviewed.

Info

All issues must be reviewed. Even non-technical issues will benefit from another team member seeing what has been done. Review is not just used to ensure correctness, it is used to ensure that at least two people are aware of what happened.

Review is a corner-stone of our process. It is a serious process and shouldn't be "eyeballed". Reviews allow team members to learn from each other. As such, it is important that if you have improvements to suggest, the code is not understandable, or you detect something wrong, you mention this in the merge request or the issue. Peer-review is the major mechanism by which team members to improve their skills day-to-day.

A review is more about learning than "checking work", it is important to take reviews as a positive process, not negative criticism. This is be especially important for junior team members or mentees.

An issue in review may be reviewed by any team member but the team member who completed the work may "@-tag" a particular person if they feel that there is someone who would be particularly familiar with the work. If the issue includes Merge Requests, the reviewer(s) will be assigned as reviewer(s) in the corresponding Merge Requests.

The reviewer(s) will take a look at the issue, any description of the work completed and any associated merge requests.

If any of the reviewers have questions or concerns, they should write them in the issue and label it as workflowRework. Even if the reviewer(s) only has a question, not any concerns, they should label the issue.

Review requires that the reviewer test the functionality. Ordinarily this is done as part of the "review required" stage and the issue can then move directly to "rework" or be closed. If the work requires that someone other than the reviewer test it and confirm that the work has been completed, the reviewer will add the additional reviewer as Merge Request reviewer or "@-tag" in the issue and explain in the issue what testing needs doing.

If all the reviewers understand what has been done and are confident that what has been done matches the issue description, they close the issue. For issues with merge requests, it is usually the case that merging the merge request will automatically close the issue.

Just after merging all the corresponding merge requests, the reviewer should deploy the new updated master branch that includes all the changes to all workspaces. In our modern repositories this will happen automatically to staging and to production by clicking a button. Where this is not automated reviewer will execute the corresponding terraform or ansible to deploy to all environments, always with the normal precautions taken when deploying, which include regression testing. The issue will be updated indicated to which workspaces it has been deployed. In cases where it can't be deployed to production or some other environments just after merging, the issue will be reopened and labeled as "workflow::needs deployment", for deploying in another occasion. The issue will be updated to indicate that deployment couldn't occur at the time of merging.

In cases where an issue can't be deployed within a reasonable timespan to production due to long blockers, coordination with other issues, downtime window required, etc. A new issue should be created for deployment purposes and added as a "chore" in the corresponding sprint where it is expected to be deployed.

Things to review when looking at issues with associated merge requests include: functionality, design, code, code style, tests, CI/CD pipelines, security advisories from GitLab CI and code quality check from GitLab CI.

Sometimes the issue will also need a separate testing step. For example, a non-trivial code change may require that a reviewer spin up a local copy of deploy the code change to the development instance. In this case the reviewer will indicate that they are testing the change in the issue. They may also do this if they are happy with the code but are blocked from testing it because, for example, the development instance is being used by another developer.