Der perfekte Workflow für Pull-Requests (als Werkzeug für Code-Reviews)?

gistfile1.txt

Pull-Requests funktionieren wunderbar für kleinere Sachen: Bugfixes, kleinere Features. 
Alles, wo die Anzahl der Changes überschaubar bleibt und sich ein Review in kurzer Zeit
gut bewerkstelligen lässt. Siehe: http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/

Die Frage ist, wie man das bei größeren Features organisiert. 

Angenommen ein Entwickler bekommt die Aufgabe, ein komplett neues Modul in einer App 
zu bauen. Bricht man die Aufgabe – was für die Reviews wichtig ist – in kleine 
Einheiten auf, hilft das sicherlich auch schon mal allgemein bei der Strukturierung.

Es ergeben sich also nun, beispielhaft, zwei Tickets: View Model bauen, View bauen.

Er erzeugt abgehend von `develop` einen Branch für das View Model und reicht den 
als PR ein. Nun kann er die View nicht ohne View Model bauen. Was also hier tun?

1) Warten, bis jemand das Review durchführt?

> Blöd, wenn es Freitagnachmittag und er gerade im Flow ist. Was für eine Verschwendung.

2) Zurück zu develop und weiterbauen?

> Geht nicht, er braucht den letzten Code-Stand.

3) Nicht von `develop`, sondern vom View Model-Branch abzweigen und den View-Branch
   aufmachen?
   
Das würde technisch gehen, birgt aber das Risiko, dass der erste PR abgelehnt oder
noch mal massiv geändert werden muss. (Gut, hier kann er die Änderungen nachher
rübermergen).

- Empfehlungen?

Einen Pull Request (im eigentlichen Sinne) würde ich nur verwenden, wenn ich (kleine) Änderungen/Verbesserungsvorschläge am Code anderer Entwickler habe. Schreibe ich eigenen Code, wie beispielsweise ein ganzes Modul, hat der Pull Request m.E. nur die Aufgabe, den neuen Code in die Master oder QA Branch zu überführen. Wenn du dem Entwickler nicht vertraust, bleibt dir nur die vollständige Review.

@forki Checkpoints: ACK. Bevor man dann aber seine PRs selbst nach develop (oder wie auch immer der integrierende gemeinsame Branch heißt) merged, würde ich eher dazu tendieren, wie von @anrichter vorgeschlagen, und auch eingangs schon als 3. Option erwähnt, den nächsten Branch vom noch nicht gereviewten und gemergten abzuzweigen. Wenn das alles zu lange dauert, wird es gammelig, das ist richtig, aber dann hat man ohnehin ganz andere Probleme. Was meinst du?

@Stoggy Das hat nichts mit Vertrauen zu tun, sondern mit Reviews nach 4-Augen-Prinzip. Das mache ich mit meinen Teams schon seit Jahren, bisher aber immer retrospektiv, nachdem Code in develeop o.ä. gelandet ist. Die Pull-Requests ermöglichen, sich die Kinder noch mal anzuschauen, bevor man sie in den Brunnen wirft. Checkpoints, wie @forki so schön schrieb.

@MikeBild Glaube wir sind da sehr nah beisammen. Wir arbeiten im Prinzip nach git-flow, d.h. wir integrieren regelmäßig auch unfertige Features nach develop, und die gehen nachher, Feature-Toggles sei Dank, auch nach master. Ich habe auch grundsätzlich nichts dagegen, einen PR als Feature-Request zu betrachten und ihn so lange offen zu lassen oder gar erst einzureichen, bis er fertig ist bzw. alle zufrieden sind. Aber ich habe Bedenken, dass so ein Changeset aus vielleicht 150 Commits und was weiß ich wie vielen Änderungen am Ende nicht mehr zu handlen sein wird. (Bzw. ist das sicher so.)

@aspnetde Dein Szenario passt nicht mehr. Ich weiß - Übertreibung macht anschaulich ;-). 150 Commits sind nach meiner Definition unmöglich! Das sind sagen wir mal ca. 14 Tage Arbeit! Sorry, das geht so nicht. Offenen Pull-Request gibt es in einem "closed project" max. 2 Tage! Deshalb ja Code-Reviews und Pull-Requests. PS: Git-Flow ist nett, kann IMHO auch zur Falle werden.

@MikeBild Och, ich mag niemandem vorschreiben, wie er committed. Manche schaffen 150 Commits auch in drei Tagen. Aber das ist auch egal, geht ja um den Umfang der Änderungen insgesamt für den PR. Ob die nun mit 25 oder 150 Commits daherkommen, ist ja wurscht.

PS: Wir haben git-flow adaptiert, funktioniert wunderbestens - ist der Startpunkt für die parallele Entwicklung mehrere Versionen inkl. allem CI und Deployment. Aber das ist ein anderes Thema.

@aspnetde Zusammengefasst: Was nicht in -sagen wir mal ca- 2 Tagen geschafft wird, muss als "progressive oder incomplete Feature" integriert werden. Das kann ja auch in ein Beta/Alpha Branch mit Continuous Delivery Pipeline sein.

@aspnetde Du musst niemanden vorschreiben wie commited wird. Jeder für sich kann ja "lokal" commiten wie er will. Meinetwegen auch 150 commits in 3 Tagen. Was remote commited ist - das zählt für mich - ergibt sich IMHO mit einer Feature-Beschreibung und Rebase von ganz allein ;-).

@aspnetde PS: sorry, nix gegen git-flow! Ist OK!

Warum nicht wie @anrichter auch schon geschrieben hat mit Feature-Branches arbeiten? Der PR wird erst erstellt, wenn das Ding gereviewt (Denglisch ist Mist) werden kann. Sprich der Entwickler ist der Meinung, das seine Arbeit erledigt, UnitTests geschrieben und der aktuelle Develop gemerged ist. Anschließend nimmt sich jemand den PR und führt das Review durch.

Ist jemand anders von diesem Feature abhängig, dann wird es entweder geschoben oder beide sprechen sich ab. Das haben wir schon so in einem 20 Mann-Team gemacht - mit guter Erfahrung.

Das machen wir so, aber je nach Umfang des Features, wird der PR halt groß und umfangreich, was das Reviewen erschwert.

Wir nutzen GitFlow. Jeder arbeitet in seinem Feature Branch. Ist der fertig, wird ein PR erstellt egal wie groß der wird.
Da die meisten PRs in nur 5 Minuten fürs Review benötigen, macht es nicht viel aus, wenn mal welche dabei sind, für die man 30 Minuten oder länger braucht. Das ist eh eher selten. Es wird meist darauf geachtet, dass die Changes klein sind (klar, geht nicht immer). Zur Not wird aber auch mal ein Feature kleiner gemacht.
Ein Paar regeln gibt es halt auch dazu:

• Jeder Reviewed jeden.
• Kein PR darf älter als 24 Stunden sein
• Ein Approval reicht, außer es werden vom Autor mehr gefordert

Das funktioniert im Großen und Ganzen sehr, sehr gut und zuverlässig

@CayasSoftware Nichts anderes machen wir. Aber das Ergebnis wird mir zu groß.

Ein paar benimmse Tippsvon @JuergenGutsch Ja, dass gefällt mir. Und BTW @forki hat natürlich recht! PR + CR hat natürlich die wichtige Aufgabe für ein paar Stunden innezuhalten und die eigene Arbeit zu kommunizieren. Deshalb auch keine meterlangen PRs, oder zu viele offene PRs. Jeder im Team ist gefragt, den anderen schnell und unkompliziert zu unterstützen.

Da bin ich dabei:

• Jeder Reviewed jeden.
• !!!Kein PR darf älter als 24 Stunden sein!!!
• Ein Approval reicht, außer es werden vom Autor mehr gefordert

@MikeBild Du arbeitest am Wochenende? Get a life! ;-)

Wir werden jetzt erst mal Folgendes machen:

• PRs bleiben so klein wie möglich
• Reviews erfolgen kontinuierlich (Bekanntmachung im Team über einen Slack-Channel (da man in GitLab nur einen Reviewer auswählen kann :()
• Sollte ein Review nicht so schnell möglich sein, wie man weitermachen möchte, wird vom Feature-Branch der nächste abgezweigt, so dass es am Ende zwei PRs gibt. Was passiert, wenn es hier mal zu Komplikationen wg. Problemen im vorangegangenen PR kommt, warten wir einfach ab.

Hallo die Herren,

ich habe nicht sämtliche Kommentare gelesen, will aber auch noch meinen Senf dazu geben: @aspnetde's ursprüngliche Frage scheint aus meiner nicht ganz zu seinen Schlüssen (https://gist.github.com/aspnetde/511c72e743fb658f71f186df12fca85d#gistcomment-1886919) zu passen ;)

Ich denke das Problem "zu großer" PRs lässt sich ganz leicht umgehen, indem man einen PR (und damit topic-Branch) für jedes Produktinkrement erzeugt. Das ist an sich gut validierbar und weil Inkremente immer Durchstiche sind damit gut per UI/API/sonstwas zu verifizieren. Will der Developer am nächsten Inkrement weiterarbeiten während er auf das Review des PR wartet erzeugt er einen neuen topic-Branch am letzten Commit des PR. Muss der PR nachgebessert werden, erfolgt ein rebase des zweiten Arbeitsgangs gegen die Korrekturen.

Wer öfter mit solchen Abhängkeiten zwischen Branches zu tun hat dem sei dieses hier empfohlen: https://github.com/greenrd/topgit

@agross Na das passt doch. Mit dem Rebase bleibt die Timeline (bei 150 commits ;-)) auch verständlich(er). Schön das jemand gleich ein Konzept, Methode und Tooling anbietet. Wir machen das -ähnlich- bisher manuell, um niemanden zu schocken. Danke! Ich schau mir das mal an.

@agross Ich brauchte irgendwas, das einen linearen Entwicklungsprozess veranschaulicht. Mir fiel dann die hypothetische test-getriebene Entwicklung des View Models ein, mit nachgelagerter Erstellung der View. Nicht, dass das irgendwie praxisnah wäre. 🙄

Aber am Ende sind wir alle beisammen.

Danke für den Austausch an die Runde 👍