Lessons learnt - Github Review Workflow

In den letzten Wochen haben wir etwas genauer auf unsere Github Workflows geschaut. Dabei sind uns einige Patterns aufgefallen, die in vielen Projekten die Zusammenarbeit einfacher gemacht haben, die wir hier teilen wollen.

Reviews, Pairing, Sparring Partner

Konvention vs. Empfehlung

Manche Teams haben zwar informell die Konvention, dass aller Quellcode nach dem vier-Augen-Prinzip entweder im Pair entwickelt wird oder durch ein Review läuft, brechen aber in der Praxis in Einzelfällen mit dieser Konvention. Soll jetzt ein vier-Augen-Prinzip über Pull Requests zwingend etabliert werden, wird transparent, wie oft von wem die Konvention nicht eingehalten wurde. Es braucht ein projektweites Verständnis, warum und in welchen Fällen ein Review gebraucht wird. Andererseits braucht es ein gemeinsames Verständnis davon in welchem Zeitrahmen mit einem Review zu rechnen ist. Hier stellt sich die Frage, welche Incentives im Team gesetzt werden, welchen Stellenwert z.B. Mentoring oder Coding einnehmen.

Kultur der Wissensweitergabe

In vielen Projekten gibt es einzelne Aspekte, bei denen sich nur wenige Entwickler auskennen. Will man für Änderungen ein vier-Augen-Prinzip etablieren, bedeutet das im Umkehrschluss, dass es für alle Aspekte mehr als einen Experten geben sollte (oder jeweils ein neuer Experte ausgebildet wird).

Damit Reviews nicht den Entwicklungsverlauf ausbremsen, braucht es engagierte Reviewer die zeitnah Feedback geben. Hilfreich hier kann es sein:

  • passende Anreize zu schaffen, Reviews zu erstellen,
  • transparent zu machen, wo viel Unterstützung in Reviews herkommt,
  • eine Rolle zu schaffen, die non-coding Aufgaben fokussiert
    angeht und diese Rolle gegebenenfalls im Team zu rotieren,
  • dringende PRs dem Team so transparent wie möglich zu machen, z.B. im passenden Slack Channel.

Außerdem sollten Reviewer nicht allein als Kontrollinstanz fungieren, sondern Sparringpartner und Unterstützer sein. Dazu gehört auch, dass Reviewer nicht ausschließlich Änderungen als Anforderungen formulieren, sondern Hilfestellung anbieten, gegebenenfalls mit dem ursprünglichen Autor gemeinsam in einer Pairing Session an Korrekturen arbeiten.

Kontinuierliche Verbesserung statt Perfektion

Dazu gehört auch, als Reviewer nicht nach dem perfekten Patch zu suchen, sondern sich bei Änderungen die Frage zu stellen, ob der damit erreichte Stand eine Verbesserung des Status Quo darstellt.

Will man den ursprünglichen PR Autor motivieren, noch einen Schritt weiter zu gehen, reicht es oft, Vorschläge als solche zu formulieren, das heißt, sie klar als optional oder als mögliches Follow-Up zu markieren. Meist genügt das als Motivation, diese Änderungen zeitnah mit umzusetzen. Selbst ein "Danke für den Patch, die Implementierung der fehlenden Option X nehme ich mir mit" kann auf PR Autor Seite motivieren, die zusätzlich notwendige Änderung doch gleich noch selbst vorzunehmen.

Spielregeln

Missverständnisse vermeiden

Gemeinsame Spielregeln sollen helfen Missverständnisse zu vermeiden. Sie sollten so übersichtlich wie möglich sein und sich auf den Normalfall konzentrieren - statt detailreich alle Randfälle abzudecken.

Spielregeln definieren

Spielregeln sollten möglichst vom Projektteam gemeinsam definiert werden. Projektlokale Spezifika sollten möglich sein, auch wenn man sich vielleicht auf eine gemeinsame Basis bereits geeinigt hat.

Hinterlegt werden können die Spielregeln in einem CONTRIBUTION.md Dokument, wo sie bei Änderungswünschen wieder über PR angepasst werden können.

Beispiele

Beispiele für Spielregeln, die Missverständnisse oder Unsicherheiten vermeiden helfen:

  • Wann darf ein PR Review Request abgelehnt werden?
  • Wann darf ein PR selbst abgelehnt werden?
  • Wieviele Reviews wollen wir je PR?
  • Gibt es davon Ausnahmen?
  • Wie stellen wir sicher, dass dringende Änderungen in der täglichen Arbeit
    zeitnah gereviewed und deployed werden?

Best Practices

In der täglichen Arbeit können kleine Angewohnheiten helfen, die Suche nach einer gemeinsamen Lösung zu vereinfachen. Diese sind oft nicht formal als Spielregeln vorgegeben, sondern werden in Form von Best Practices gelebt:

"Das könnte man aber besser machen"

Wenn man sich beim Review darauf konzentriert, den Status Quo zu verbessern, auch wenn das nicht zur perfekten Lösung führt, ist es nicht unwahrscheinlich, dass der Eindruck, nur "halb fertig geworden zu sein" zurückbleibt.

Hier kann es helfen, eine Adresse für offene Enden zu finden. Das kann ein neues Issue sein, eine Rolle im richtigen Kreis, die sich das Thema mitnimmt, ein Eintrag im Backlog oder ein Eintrag in der Roadmap.

Eine andere Alternative kann es sein, als Reviewer in Abstimmung mit dem PR Autor gewünschte Verbesserungen selbst direkt auf dem PR Branch vorzunehmen.

"Bitte nicht mergen"

Release early - release often bleibt auch beim asynchronen Arbeiten über Pull Requests wichtig. Hilfreich hier kann es sein, Änderungen, die noch im experimentellen Stadium sind, schonmal als work in progress zu teilen. Wichtig dabei sind klare Verabredungen im Team, wie solche Änderungen zu markieren sind, damit sie nicht aus Versehen im Repository landen.

Technische Unterstützung ist hier möglich in Form von Bots, die verhindern, dass PR mit der Abkürzung WIP im Titel gemerged werden.

Asynchron ist nicht alles

Schriftliche Kommunikation ermöglicht asynchrones und verteiltes Arbeiten, führt zu mehr Nachvollziehbarkeit. Der Nachteil ist der Verlust an Kommunikationsbandbreite. Das wird umso deutlicher, je länger Diskussionen geführt werden. PR Reviews, die viel Kommunikation brauchen sollten nach Möglichkeit in ein anderes Medium mit mehr Bandbreite gezogen werden. Bei komplexeren Fragen kann es hilfreich sein, gemeinsam zum Pairing überzugehen, um Ungereimtheiten zu beseitigen.

Auch Beiträge, die negativ wahrgenommen werden, sind oft anders gemeint, als man es selbst beim Lesen verstanden hat. Der Wechsel auf ein Kommunikationsmedium mit mehr Bandbreite - im Idealfall über einer Tasse Tee kann hier Wunder wirken.

Sparringpartner suchen

Wird eine Änderung eingereicht, hat es sich bewährt, direkt einen passenden Reviewer anzusprechen und auszuwählen. Damit ist klar, bei wem der Ball für die weitere Arbeit liegt.

Es hat sich als günstig herausgestellt, diesen Sparringpartner bereits vor Beginn der eigentlichen Arbeit anzusprechen und mit ins Thema reinzunehmen.

Viele Pull Requests zusammenhalten

Große Mengen an Code Änderungen lassen sich nur sehr zeitaufwändig reviewn. Es hat sich bewährt, große Änderungen in kleine Schritte aufzuteilen - dann können im Idealfall sogar unterschiedliche Personen bei unterschiedlichen Teilabschnitten helfen. Um dennoch ein koheräntes Bild zu behalten hilft es, am Anfang ein Issue mit der Vision hinter der gesamten Änderung zu erstellen.

Einzelne Teams bei Europace experimentieren auch mit der Möglichkeit, PRs auf PRs zu erstellen. Erste Erfahrungen sind hier vielversprechend.

Bei der Entscheidung, ob eine Änderung lieber hinter einem Feature Flipper versteckt in vielen, kleinen, unabhängigen Schritten live geht - oder als ein long-running WIP Pull Request: Ist es für die weitere Entwicklung hilfreich, die Änderung schonmal auf Prod zu haben?

Und nach dem Spielen wird aufgeräumt

git squash und git rebase können hilfreich sein, auch im boyscouting Modus erstellte Änderungen in eine lesbare Änderungshistorie zu überführen. Im Reviewprozess sollte das aber möglichst nicht gemacht werden, will man vermeiden, dass der Reviewer sich mit jedem neuen Zyklus nochmal die kompletten Änderungen anschauen muss.

Der Ton macht die Musik

Selbst wenn alle Beteiligten in einem Raum sitzen: Durch die schriftliche Kommunikation geht Bandbreite verloren. Inklusive Formulierungen können hier unterstützen. Ebenfalls hilfreich: Die Erkenntnis, dass das was aufgeschrieben wird, nicht nur für andere, sondern auch für das eigene zukünftige Selbst erstellt wird.

Und wie sieht jetzt der Github Workflow anderswo aus?