Revue de code

Comment faire des retours constructifs

Le reviewer/mainteneur s’engage à effectuer la revue de code rapidement, c’est aussi à lui qu’appartient de fermer les « request changes », de bien vérifier l’historique des commits, et de fusionner la « pull request » s’il en a les droits.

Nous vous soumettons quelques conseils et recommandations qui nous semblent pertinentes pour une revue de code humaine, pertinente et enrichissante pour tous ses contributeurs :

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

Cycle de review

Une revue de code est un processus itératif. Pour la fluidité d’une review, il est impératif de configurer correctement ses notifications github.

Il est conseillé de configurer les dépôts OSRD en “Participating and @mentions”. Cela permet d’être notifié d’activités uniquement sur les issues et PR auxquelles vous participez.

sequenceDiagram
  actor A as Auteur PR
  actor R as Reviewer/mainteneur

  A->>R: Demande une review en notifiant spéciquement quelques personnes
  R->>A: Répond à la demande par oui ou non

  loop Boucle entre auteur et reviewer
    R-->>A: Commente, demande des changements
    A-->>R: Répond à chaque commentaire/demande de changement
    A-->>R: Corrige le code si nécessaire dans des « fixups » dédiés
    R-->>A: Vérifie, teste, et commente à nouveau le code
    R-->>A: Résout les conversations/demandes de changement le cas échéant
  end

  A->>R: Rebase si nécessaire
  R->>A: Vérifie l'historique des commits
  R->>A: Approuve ou ferme la PR
  Note left of R: Et fusionne si mainteneur

Les mainteneurs sont automatiquement notifiés par le système de CODEOWNERS. L’auteur d’une PR est responsable de faire avancer sa PR dans le processus de review (quitte à notifier manuellement un mainteneur).

La pyramide de la revue de code

Script pour le test d’une PR

Lors de la revue d’une PR, il est utile de tester les changements en démarrant une instance de l’application OSRD basée sur la branche de la PR.

Un script est disponible pour lancer automatiquement une instance séparée et dédiée de l’application en utilisant le numéro de la PR. Le script utilise les images Docker déjà construites par la CI et lance l’application de manière isolée. Cela permet d’exécuter les deux instances en parallèle, sans conflits (idéal pour comparer les modifications, par exemple).

De plus, vous pouvez fournir une sauvegarde de la base de données, que le script chargera directement dans l’application.

L’application sera lancée sur le port 4001. Vous pouvez y accéder en suivant : http://localhost:4001/

Commandes Disponibles :

./scripts/pr-tests-compose.sh 8914 up : Télécharge les images CI générées pour la PR #8914 et lance l’application.
./scripts/pr-tests-compose.sh 8914 up-and-load-backup ./path_to_backup : Télécharge les images pour la PR #8914, restaure les données à partir de la sauvegarde spécifiée, et démarre l’application.
./scripts/pr-tests-compose.sh down : Arrête l’instance de test de l’application pour la PR #8914.
./scripts/pr-tests-compose.sh down-and-clean : Arrête l’instance de test et nettoie l’ensemble des volumes docker de l’instance (base de donnée PG, cache Redis, RabbitMQ) pour éviter tout effet de bord.

Accès aux Services :

À l’exception du serveur frontend, tous les services sont disponibles sur localhost, avec un léger ajustement de port (pour éviter les conflits avec l’environnement de développement) : pour la liste des ports, reportez-vous au fichier docker compose dédié.