Keyboard shortcuts

Press ← or → to navigate between chapters

Press S or / to search in the book

Press ? to show this help

Press Esc to hide this help

Review Guide

Scope

This document applies to all repositories in the Tarook GitLab group.

Goals of the Review

  • Improve code quality and maintainability
  • Spread knowledge about how the system works between code authors and maintainers
  • Catch errors which cannot be caught by automation early on

Non-goals of the Review

  • Waste of time
  • Substitute for unit tests

Dramatis personae

  • Reporter: One or more people who discover an Issue within the product (may be a missing feature, a bug, a missing enhancement or similar)
  • Developer: One or more people who work on resolving Issues within the product.
  • Reviewer: A person who reviews the work done by a Developer.

How to Review

During review, you need to check for the following things:

  1. Is the Merge Request description and title good?

    The title should explain what has changed and the body should completely explain why it changed. During the review, no information from the commit messages should be necessary to understand what is going on.

    The title MUST NOT match /Resolve ".*"/.

  2. Is the pipeline green?

    If the pipeline is not green and the Developer has no convincing argument why you should still review the code, stop here and hand the MR back to the Developer for fixing. Don’t forget to tell them why.

  3. Is the code readable, formatting wise?

    Even though we use an automated style checker, some things cannot be enforced by that and there will always be judgement-call edge cases. Ensure that all code is reasonably formatted and can be read.

  4. Is the code understandable, content wise?

    A review should not take tremendous amounts of time per line. The code needs to be understandable, even for people not intimately familiar with the system it belongs to. If it is not clear why the code operates or how the code operates, it needs to be explained by the author – preferably in a comment, but in some cases in the commit messages, too.

  5. Have tests been added?

    All functions should have unit tests completely covering them (use the coverage markers displayed by gitlab as a first line of defense here). New features in operators should come with API-level semi-integration tests. If possible, new services should be smoke-tested in the integration test in the CI.

  6. Are there any logic errors?

    That one speaks for itself. With the information from the previous points you should be able to spot any obvious logic errors.

  7. Have the docs been updated?

    • If the code introduces a new operator or dependency, that must be added to the dev start. It must also be added to the docs.
    • If the code introduces a new concept, a piece of documentation must be added.
    • Environment variables, Kubernetes labels, annotations and taints need to be documented.
    • Classes and non-private functions and methods need docstrings and should be linked from the docs.

  8. Are the commit messages good?

    A good commit message consists of a subject line summarizing the change followed by one or more paragraphs which explain why the change was necessary. It should contain a link to an issue which has more details and gives more context. They must contain all information given in the MR description.

    The guide "How to Write a Git Commit Message" should be followed.

Add comments for anything wrong. If you think that some issues you found will cause changes invalidating a later step of the review, you may point that out and stop the review there to avoid doing work twice.

Hand the MR back to the author and let them apply the fixes. Once the MR satisfies you, add a comment matching the following template, crossing off everything you validated and adding a note explaining why you did not check a specific thing if you skipped it:

- [ ] Pipeline green
- [ ] Code is readable
- [ ] Code is understandable
- [ ] Tests have been added and cover the added functionality as is reasonable
- [ ] Docs have been updated
- [ ] Commit messages look good
- [ ] **LGTM**

You may then approve the MR. If sufficiently privileged, you can hit the Merge button, too.

Note: The comment and approval will appear in the MR history, which allows someone who looks at it later to validate that no changes have been made after the review stage has been completed.