Review process

How to give useful feedback

The reviewer/maintainer undertakes to carry out the review quickly, and is also responsible for closing request changes, check commit history and quickly merge the pull request if allowed.

We propose you a few tips and recommendations that we think are relevant to a human, relevant and rewarding code review for all contributors:

How to Make Your Code Reviewer Fall in Love with You? by Michael Lynch.
How to Do Code Reviews Like a Human? by Michael Lynch.

Review cycle

A code review is an iterative process. For a smooth review, it is imperative to correctly configure your github notifications.

It is advisable to configure OSRD repositories as “Participating and @mentions”. This allows you to be notified of activities only on issues and PRs in which you participate.

Maintainers are automatically notified by the CODEOWNERS system. The author of a PR is responsible for advancing their PR through the review process and manually requesting maintainer feedback if necessary.

sequenceDiagram
  actor A as PR author
  actor R as Reviewer/Maintainer

  A->>R: Asks for a review, notifying some people
  R->>A: Answers yes or no

  loop Loop between author and reviewer
    R-->>A: Comments, asks for changes
    A-->>R: Answers to comments or requested changes
    A-->>R: Makes necessary changes in dedicated "fixups"
    R-->>A: Reviews, tests changes, and comments again
    R-->>A: Resolves requested changes/conversations if ok
  end

  A->>R: Rebase and apply fixups
  R->>A: Checks commits history
  R->>A: Approves or closes the PR
  Note left of R: & Merges if maintainer

The code review pyramid

Script for testing a PR

When reviewing a PR, it is useful to test the changes by starting an instance of the OSRD app based on the PR branch.

A script is available to spin up a separate and dedicated app instance using the PR number. The script uses the Docker images already built by the CI and launches the app, running in isolation. This allows you to run both instances simultaneously without conflicts (ideal for comparing changes, for example).

Additionally, you can specify a database backup, which the script will load directly into the app.

The app will be launched on the 4001 port. You can access it at: http://localhost:4001/

Available Commands:

./scripts/pr-tests-compose.sh 8914 up: Downloads the CI-generated images for PR #8914 and launches the application.
./scripts/pr-tests-compose.sh 8914 up-and-load-backup ./path_to_backup: Downloads the images for PR #8914, restores data from the provided backup, and starts the application.
./scripts/pr-tests-compose.sh down: Shuts down the test application instance for PR #8914.
./scripts/pr-tests-compose.sh down-and-clean: Shuts down the test instance and cleans all the instance’s docker volumes (PG data, Redis cache, RabbitMQ) to prevent any side-effects.

Accessing Services:

Apart from the frontend server, all localhost services are available on localhost, with a minor port adjustment (to avoid conflicts with the dev environment): for a list of common ports, have a look at the dedicated docker-compose file.