GitLab Guidelines

Introduction¶

CIA Linux GitLab Project Setup¶

Project Settings¶

All CIA Linux-owned projects should have the following base configurations:

1. GitLab Project:

   • Project visibility should be "Internal" or "Private" by default, in order to encourage transparency, increase communication, and build trust with other ITS units.
   • CIA-owned projects should live under the CIA group unless it is a shared project, such as a container, an Ansible role, or equivalent.
   • CIA-owned projects should have the @cia-linux group added as "Owners" or "Maintainers".

   Note

   The goal of keeping projects visible to the other members of GitLab is to increase visbility and facilitate collaboration. This must of course be balanced with security. As such, we tend to encourage the following:

   • Mirrors of Open Source sotware should be "Public".
   • Projects that are of benefit to other IT units at McGill should be "Public".
   • Projects that detail how McGill systems are configured or how McGill units operate should be "Internal".
   • If the project contains sensitive content that is not suitable to be shared with other GitLab users, it should be "Private".
     • Note that even in the case of "Private" repositories, no secrets should be commited to your git repository unencrypted.

   Tip

   Projects can not be less restrictive than the group they are in. For example, the CIA group is set to "Internal", so no project in that group can be "Public".

2. Git Repository:

   • The trunk of the project should be named main.

   Note

   Historical projects may be named master, but an issue should be created to change the default branch to main to be in-line with the rest of our projects and the general modernisation of the git ecosystem.

   • No one should be allowed to push to the trunk (defined as main or master)
   • Only maintainers should have permissions to merge projects into the trunk.
   • A CODEOWNERS should be present in all repositories, defining basic ownership. More details on CODEOWNERS below.
   • Merge Requests must be defined to require CODEOWNER approval.

3. CODEOWNERS:

   • GitLab sub-groups should be created for CIA Subject Matter Experts (SME) under the @cia-sme group. For example, for Ansible, we have @cia-sme/ansible.
   • For projects exclusive to CIA Linux, the CODEOWNERS should specify the SME group responsible for approvals. If a group does not exist, consider if creating one makes sense. If not, use the McGill email addresses of those responsible for the code.
   • There should be at least 2 owners for any given section. It is assumed that there are at least 2 SMEs in any group, in which case a single SME group can be added when appropriate.

4. CI:

   • Any project that contains code or scripts should at the very miniumum leverage GitLab CI to run basic lint and/or syntax checks on all merge requests. Please consider contributing lint tests configurations to the GitLab CI Template project, and then including them in your project.
   • Whenever possible more advanced tests should be included. As with linting please consider contributing the GitLab CI configurations to the CI Template project so that they may benefit the entire ITS community.
   • All projects should include Secret Detection, as unencrypted secrets should never be stored in a git repository. This feature is not foolproof, but does add an additional layer of protection.
   • Where appropriate, GitLab CI should be leveraged for continous deployment.

Roadmap¶

• CIA Linux will investigate a GitLab project that will use GitLab CI to manage project configurations in order to harmonize the above detailed requirements. Such a project will use either Ansible or Terraform.

Code Review Guidelines¶

This guide contains advice and best practices for performing code review, and having your code reviewed.

All merge requests for CIA Linux projects, whether written by a CIA Linux team member or another contributor, must go through a code review process to ensure the code is effective, understandable, maintainable, and secure.

Getting Merge Request Reviewed, Approved, and Merged¶

You are strongly encouraged to get your code reviewed by a reviewer as soon as there is any code to review, to get a second opinion on the chosen solution and implementation, and an extra pair of eyes looking for bugs, logic problems, or uncovered edge cases.

The default approach is to choose a reviewer from your group or team for the first review. This is only a recommendation and the reviewer may be from a different team. However, it is recommended to pick someone who is an SME. If your merge request touches more than one subject matter (for example, Linux server configurations and Database schemas), ask for reviews from an expert from each subject.

You can read more about the importance of involving reviewer(s) in the section on the responsibility of the author below.

If you need some guidance (for example, it’s your first merge request), feel free to ask one of the merge request coaches by mentioning @cia-sme/merge-request-coaches in your merge request.

Depending on the areas your merge request touches, it must be approved by one or more SME:

For approvals, we use the approval functionality found in the merge request widget. For reviewers, we use the reviewer functionality in the sidebar. Reviewers can add their approval by approving additionally.

When more than one approval is required, the last SME or qualified approver to make the approval is responsible to complete the merge.

Cross-Team Approval Workflow¶

How other teams choose to manage their code reviews and merge requests is left up to them, with the exception that any GitLab projects that deploy to Linux servers with root-level access must include @cia-linux in the review and approval process. This should be enfoced on the project by limiting direct-pushes to any branch that deploys to any server environment greater than Development, and requring a CODEOWNERS file with the following configuration line:

* @cia-linux

CIA Linux memebers will not be responsible for merges, and we may offer advice on areas where we have expertise. However, we will not block any merge requests unless there is a security or operational configuration concern with the proposed solution.

Subject Matter Experts¶

Subject matter experts (SME) are team members who have substantial experience with a specific technology, product feature or area of the codebase. Team members are encouraged to self-identify as SMEs and add the subject to their team profile.

When self-identifying as an SME, it is recommended to assign the MR changing the cia-team.yml to be merged by an already established SME or a corresponding manager.

We make the following assumption with regards to automatically being considered a domain expert:

• Team members working in a specific stage/group (e.g. create: source code) are considered domain experts for that area of the app they work on
• Team members working on a specific feature (e.g. search) are considered domain experts for that feature.

We default to assigning reviews to team members with domain expertise. When a suitable domain expert isn’t available, you can choose any team member to review the MR, or simply follow the Reviewer roulette recommendation (when it becomes available).

The responsibility of the merge request author¶

The responsibility to find the best solution and implement it lies with the merge request author. The author stays assigned to the merge request as the assignee throughout the code review lifecycle.

Before requesting a review from a maintainer to approve and merge, they should be confident that:

• It actually solves the problem it was meant to solve.
• It does so in the most appropriate way.
• It satisfies all requirements.
• There are no remaining bugs, logical problems, uncovered edge cases, or known vulnerabilities.

The best way to do this, and to avoid unnecessary back-and-forth with reviewers, is to perform a self-review of your own merge request, following the Code Review guidelines.

To reach the required level of confidence in their solution, an author is expected to involve other people in the investigation and implementation processes as appropriate.

Avoid:

• Adding TODO comments (referenced above) directly to the source code unless the reviewer requires you to do so. If TODO comments are added due to an actionable task, include a link to the relevant issue.
• Adding comments which only explain what the code is doing. If non-TODO comments are added, they should explain why, not what.
• Requesting maintainer reviews of merge requests with failed tests. If the tests are failing and you have to request a review, ensure you leave a comment with an explanation.
• Excessively mentioning maintainers through email or MS Teams (if the maintainer is reachable through Teams). If you can’t add a reviewer for a merge request, @ mentioning a maintainer in a comment is acceptable and in all other cases adding a reviewer is sufficient.

This saves reviewers time and helps authors catch mistakes earlier.

The responsibility of the reviewer¶

Review the merge request thoroughly. When you are confident that it meets all requirements, you should:

• Click the Approve button.
• @ mention the author to generate a to-do notification, and advise them that their merge request has been reviewed and approved.
• Request a review from a maintainer. Default to requests for a maintainer with SME, however, if one isn’t available or you think the merge request doesn’t need a review by an SME, then approve and merge the work, skipping the next step.
• Remove yourself as a reviewer.

Reviewing a merge request¶

Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then:

• Try to be thorough in your reviews to reduce the number of iterations.
• Communicate which ideas you feel strongly about and those you don’t.
• Identify ways to simplify the code while still solving the problem.
• Offer alternative implementations, but assume the author already considered them. ("What do you think about using a custom validator here?")
• Seek to understand the author’s perspective.
• If you don’t understand a piece of code, say so. There’s a good chance someone else would be confused by it as well.
• Ensure the author is clear on what is required from them to address/resolve the suggestion.
  • Consider using the Conventional Comment format to convey your intent.
  • For non-mandatory suggestions, decorate with (non-blocking) so the author knows they can optionally resolve within the merge request or follow-up at a later stage.
  • There’s a Chrome/Firefox add-on which you can use to apply Conventional Comment prefixes.
• Ensure there are no open dependencies. Check linked issues for blockers. Clarify with the author(s) if necessary. If blocked by one or more open MRs, set an MR dependency.
• After a round of line notes, it can be helpful to post a summary note such as "Looks good to me", or "Just a couple things to address."
• Let the author know if changes are required following your review.

Roadmap¶

• CIA Linux hopes to leverage Danger Systems to automatically and pseudo-randomly assign reviewers to projects in the future. See: https://docs.gitlab.com/ee/development/dangerbot.html