Skip to content

Commit

Permalink
docs: update review process
Browse files Browse the repository at this point in the history
- Add details on the use of github notifications

Signed-off-by: Florian Amsallem <[email protected]>
  • Loading branch information
flomonster committed Oct 4, 2024
1 parent 42eabac commit ae884e8
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 22 deletions.
4 changes: 1 addition & 3 deletions content/docs/guides/contribute/code-review.en.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ We propose you a few tips and recommendations that we think are relevant to a hu

{{% include "./review-process.en.md" %}}

> If the reviewer is not maintainer, the PR's author has the responsability to contact a maintainer to merge the PR. In special cases (especially near feature freeze), maintainer should be found early.
## The code review pyramid

{{< figure src="/images/docs/contribute/code_review_pyramid.svg" link="https://www.morling.dev/blog/the-code-review-pyramid/">}}
{{< figure src="/images/docs/contribute/code_review_pyramid.svg" link="https://www.morling.dev/blog/the-code-review-pyramid/">}}
4 changes: 1 addition & 3 deletions content/docs/guides/contribute/code-review.fr.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ Nous vous soumettons quelques conseils et recommandations qui nous semblent pert

{{% include "./review-process.fr.md" %}}

> Si la personne qui fait la revue de code n'est pas mainteneur, c'est à l'auteur de PR de trouver un mainteneur pour pouvoir fusionner sa PR avec le code. Dans certains cas particuliers (notamment juste avant le gel des fusions en fin d'itération), le mainteneur doit être averti plus tôt.
## La pyramide de la revue de code

{{< figure src="/images/docs/contribute/code_review_pyramid.svg" link="https://www.morling.dev/blog/the-code-review-pyramid/">}}
{{< figure src="/images/docs/contribute/code_review_pyramid.svg" link="https://www.morling.dev/blog/the-code-review-pyramid/">}}
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ because you are not just pushing new commits, you are pushing changes to existin

3. **If you believe somebody forgot to review / merge your change, please speak out, multiple times if needs be.**

## Suggested workflow

{{% include "../review-process.en.md" %}}

*[Finally continue towards tests ‣]({{< ref "tests">}})*
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ L'auteur d'une _pull request (PR)_ est responsable de son « cycle de vie ». Il
- N'importe qui peut donner son avis.
- Il est nécessaire d'obtenir l'approbation d'un [contributeur familier avec le code](https://github.com/osrd-project/osrd/blob/dev/.github/CODEOWNERS).
- Il est d'usage de prendre en compte tous les commentaires critiques.
- Les commentaires sont souvent écrits dans un style plutôt direct, dans le soucis de collaborer efficacement vers une solution acceptable par tous.
- Les commentaires sont souvent écrits dans un style plutôt direct, dans le souci de collaborer efficacement vers une solution acceptable par tous.
- Une fois que tous les commentaires ont été pris en compte, un mainteneur intègre le changement.

> Sur les PR conséquentes et vouées à évoluer dans le temps, conserver les _corrections_ suite à la
Expand All @@ -41,9 +41,7 @@ complète (demandez de l'aide au besoin) :
[`git push --force-with-lease`](https://git-scm.com/docs/git-push#Documentation/git-push.txt---no-force-with-lease)
car vous ne poussez pas seulement de nouveaux commits, mais bien un changement des commits existants.

3. **N'hésitez pas à relancer vos interlocuteurs, plusieurs fois si besoin : vous êtes responsable de la vie de votre _pull request_**.

## Proposition de fonctionnement
3. **N'hésitez pas à relancer vos interlocuteurs, plusieurs fois si besoin est : vous êtes responsable de la vie de votre _pull request_**.

{{% include "../review-process.fr.md" %}}

Expand Down
15 changes: 10 additions & 5 deletions content/docs/guides/contribute/review-process.en.md
Original file line number Diff line number Diff line change
@@ -1,23 +1,28 @@
Here's a suggested workflow.
## Review cycle

**It can be useful to communicate via instant messaging (Matrix, Slack, etc.) in order to guarantee the smooth flow of PR validation.**
A code review is an iterative process.
For a smooth review, it is imperative to [correctly configure your github notifications](https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-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.
```mermaid
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
Expand Down
15 changes: 10 additions & 5 deletions content/docs/guides/contribute/review-process.fr.md
Original file line number Diff line number Diff line change
@@ -1,25 +1,30 @@
Voici une proposition de flux de travail.
## Cycle de review

**Il peut être utile de communiquer par messagerie instantanée (Matrix, Slack, etc.) afin de garantir le fonctionnement du flux de la validation d'une PR.**
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](https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-notifications).

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.

```mermaid
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).

0 comments on commit ae884e8

Please sign in to comment.