Workflow zur Codeüberprüfung, in dem der Autor der Pull-Anforderung zusammengeführt werden muss

15

Mehrere Teams in meinem Unternehmen üben einen Code-Überprüfungs-Workflow aus, den ich noch nie zuvor gesehen habe. Ich versuche, das Denken dahinter zu verstehen, mit der Idee, dass es Wert ist, das gesamte Unternehmen konsistent zu machen. (Ich trage zu mehreren Codebasen bei und wurde durch die Unterschiede in der Vergangenheit in Mitleidenschaft gezogen.)

  1. Der Autor des Codes sendet eine Pull-Anfrage
  2. Der Prüfer überprüft den Code
    • Wenn der Prüfer zustimmt, hinterlässt er einen Kommentar wie folgt: "Sieht gut aus, Sie können ihn gerne zusammenführen."
    • Wenn der Prüfer Bedenken hat, hinterlässt er einen Kommentar wie "Bitte beheben Sie kleinere Probleme X und Y und führen Sie sie dann zusammen." (Bei größeren Änderungen kehren Sie zu Schritt 2 zurück.)
  3. Der Codeautor nimmt bei Bedarf Änderungen vor und führt dann seine eigene Abrufanforderung zusammen

Ich habe folgende Bedenken:

  • Im Fall der Genehmigung in Schritt 3 wird durch diesen Workflow ein scheinbar unnötiger Hin- und Rückflug zum Pull-Request-Autor erstellt. Der Prüfer, der sich den Code bereits ansieht, könnte ihn einfach sofort zusammenführen.

  • Im Falle von Änderungen, die in Schritt 3 angefordert wurden, liegt die Agentur, die die Pull-Anforderung zusammenführt, jetzt ausschließlich beim PR-Autor. Niemand außer dem Autor wird sich die Änderungen vor dem Zusammenführen ansehen.

Welche weiteren Vor- oder Nachteile hat dieser Workflow? Ist dieser Workflow bei anderen Entwicklungsteams üblich?

Aednichols
quelle
5
Haben Sie die Mitarbeiter Ihrer Organisation gefragt, warum sie diesen speziellen Workflow gewählt haben? Sie sind wahrscheinlich besser in der Lage, die relevanten Vorzüge zu beleuchten als wir. Wir würden nur spekulieren.
Robert Harvey
1
Was passiert in Ihrer Organisation, wenn der Prüfer "Bitte beheben Sie das Hauptproblem X" schreibt ?
Doc Brown
8
Meiner Erfahrung nach ist es am besten, wenn der ursprüngliche Autor die Zusammenführung durchführt, falls ein Zusammenführungskonflikt gelöst werden muss. Der ursprüngliche Autor ist in der Regel derjenige, der am besten in der Lage ist, herauszufinden, wie ein Zusammenführungskonflikt gelöst werden kann.
17 von 26
Ich wäre gespannt auf die Logik hier. Sie sollten Ihre Kollegen fragen und es als Selbstantwort aufschreiben - es könnte hier einen sehr guten Denkprozess oder eine gute Begründung geben. Ich bin einfach nicht in der Lage, schnell eine zu finden.
Thomas Owens

Antworten:

21

Im ersten Fall ist es normalerweise eine Höflichkeit. In den meisten Organisationen starten Zusammenschlüsse eine Reihe automatisierter Tests, die bei Fehlschlagen umgehend behoben werden müssen. Insbesondere wenn es zwischen dem Einreichen eines Pull-Antrags und der Überprüfung zu einer erheblichen Verzögerung kam, ist es höflich, zuzulassen, dass er in den Zeitplan des Autors aufgenommen wird, damit er Zeit hat, sich mit unerwarteten Ausfällen zu befassen. Der einfachste Weg, dies zu tun, ist, sie selbst zusammenführen zu lassen.

Manchmal werden dem Autor später auch Gründe dafür bekannt, dass eine Pull-Anfrage noch nicht zusammengeführt werden sollte. Möglicherweise hat die PR eines anderen Entwicklers eine höhere Priorität und würde Konflikte verursachen. Vielleicht dachte sie an einen ungedeckten Anwendungsfall. Möglicherweise hat ein Überprüfungskommentar ein Brainstorming ausgelöst, das einer weiteren Untersuchung bedarf, bevor das Problem vollständig gelöst ist. Der Autor kennt den Code am besten, und es ist sinnvoll, ihm das letzte Wort darüber zu geben, wann er zusammengeführt wird.

Beim zweiten Punkt geht es nur um Vertrauen. Wenn Sie nicht darauf vertrauen können, dass andere Personen kleinere Probleme beheben, ohne dass dies überprüft wird, sollten sie nicht für Sie arbeiten. Wenn das Problem so groß ist, dass nach dem Fix eine weitere Überprüfung erforderlich ist, sollten Sie den Überprüfern vertrauen, dass sie eine weitere Überprüfung anfordern.

Abgesehen davon füge ich gelegentlich die Pull-Anfragen anderer Autoren zusammen, aber es sind normalerweise entweder sehr einfache Änderungen oder externe Quellen, bei denen ich persönlich die Verantwortung für das Durchführen von Fehlern bei der Testautomatisierung übernehme.

Karl Bielefeldt
quelle
2
Klingt so, als gäbe es da draußen mehr Abwechslung als ich mir vorgestellt hätte. Meine Erfahrung mit automatisierten Tests hat gezeigt, dass Tests vor und nicht nach dem Zusammenführen für den Zweig ausgeführt werden und somit eine Voraussetzung für das Zusammenführen sind. Ich habe auch nicht überprüfte "kleinere" Korrekturen gesehen, einschließlich meiner eigenen, die Fehler verursachen.
Aednichols
2
Tests werden häufig sowohl als Nachbedingung als auch als Vorbedingung ausgeführt. Es ist durchaus möglich, dass Änderungen von mehreren Zweigen auf nicht offensichtliche Weise zu Konflikten führen, die nicht als Codekonflikt angezeigt werden und zu einem Fehlschlagen der Tests führen. An meinem Arbeitsplatz benötigen wir eine Zweigstelle, die mit der Basiszweigstelle auf dem neuesten Stand ist, sowie alle Tests, die bestanden werden, bevor sie zum Zusammenführen in Frage kommt. Wenn diese beiden Bedingungen erfüllt sind, erfolgt das Zusammenführen automatisch. Wir hatten nicht immer diese erste Bedingung - bevor wir tatsächlich Probleme hatten, die wir selten meistern konnten, obwohl jeder Zweig einzeln bestanden hatte
Matthew Scharley
3

Es ist mein bevorzugter Workflow in kleinen Teams, wenn der Erstautor seine eigene Pull-Anfrage zusammenführt. Zusätzlich zu den bereits erwähnten technischen Vorteilen (zum Beispiel bei der Lösung von Zusammenführungskonflikten) denke ich, dass dies einen Mehrwert auf kultureller Ebene darstellt: Es schafft ein Gefühl von Eigenverantwortung.

I angegebenen Anfang Autor für den (seltenen) Fall , wenn ein anderer Entwickler Commits zur offenen Pull - Anforderung hinzufügen würde (den Work-in-progress Zweig ziehen und sie zurückzudrängen). Dies kommt nicht oft vor und resultiert aus einem persönlichen Gespräch oder über Slack: Diese zusätzlichen Commits (von jemand anderem) sollten nicht überraschend dort landen! In diesem Zusammenhang von Anfang Autor, meine ich denjenigen, der die Pull - Anforderung vorgelegt.

mkcor
quelle
2

In meiner Organisation sind wir ziemlich neu in Bezug auf Pull-Anfragen und Ihre Frage habe ich mir selbst gestellt.

Eine Bemerkung, die ich hinzufügen möchte: In einigen Tools (wir verwenden TFS) ist möglicherweise ein Arbeitselement mit der Pull-Anforderung verknüpft.

Wenn dies der Fall ist, wird es ein bisschen mühsam, den Überblick zu behalten, wann der Prüfer die Zusammenführung durchgeführt hat. In diesem Szenario muss der Entwickler den PR erneut aufrufen, den Fehler oder die Änderungsanforderung öffnen und als "behoben" markieren. Wenn wir es zu früh als "behoben" markieren, glauben die Tester, dass der Fix bereits Teil des aktuellen Builds ist.

TFS 2017 hat die Umsetzung der Pull-Anfrage verbessert. Jetzt kann der Entwickler anfordern, dass die Pull-Anforderung automatisch zusammengeführt wird, wenn alle Bedingungen erfüllt sind (kein Zusammenführungskonflikt, Genehmigung durch Prüfer und keine fehlerhaften Builds). YMMV.

9Rune5
quelle
1

Auf diese Weise hat der Autor die Möglichkeit, seine Meinung über seine Branche zu ändern. Vielleicht hat er etwas herausgefunden, das auf eine andere Art und Weise getan werden sollte, und die zusätzlichen Kosten erneut zur Überprüfung bereitgestellt.

gnasher729
quelle