O pull-requestach slow kilka

Padlo dzis w internetach bardzo ciekawe pytanie - jak wyglada proces tworzenia/review pull request-ow w zespolach w ktorych pracujemy.

TLDR: Jest tylko jedna zasada jesli chodzi o review PRow - don’t be an asshol!

Pracowalo sie tu i tam, robie to juz dosc dlugo wiec postanowilem sie wypowiedziec (nie madrowac). To sa moje doswiadczenia i opinie - a kazdy ma swoje wlasne doswiadczenia i opinie ;)

W pracy programisty tworzenie oraz sprawdzanie pull request-ow to cos co robimy codziennie (zakladam, ze nie pracujesz sam - co nie wyklucza tworzenia PRs i ze uzywamy GITa).

Pracowalem nad projektami gdzie okresami pracowalem sam nad projektem - i nawet wtedy tworzylo sie PR ;) Wroce do tego tematu pozniej.

Slyszalem o ludziach co robia commity prosto do mastera - ja osobiscie nie mialem okazji pracowac w takim zespole - ale kazdy zespol ma swoje workflow/procesy itd - punkt widzenia zalezy od punktu siedzenia.

Moze zaczniejmy moze najpierw od tego co to jest PULL REQUEST. Pull request to jesli pamiec mnie nie myli wynalazlek githuba a nie cos co instanio w GIT-ie od poczatku.

Pull Request to lista commitow (zmian w kodzie), ktore chcemy “zmergowac” (nie wazne czy uzywamy strategi merge/squash) z innego brancha do brancha glownego (nie wazne czy uzywamy master/ develop).

Ale po co tworzyc PRs? Glownie po to zeby moc opakowac jakas konkretna zmiane w cos co moze byc przejrzane przez innego programiste lub nas samych.

Rozne zespoly maja rozne procesy jesli chodzi o PR - na to czym jest PR i jak wyglada jego review moze miec wplyw duzo czynnikow. Czy mamy automatyczne testy, czy mamy narzenia do lintowani, czy mamy wiele srodowisk (env per branch vs stage only). Proces sprawdzenia PR moze rowniez wygladac roznie, ze wzgledu na to czego uzywamy jako zespol - github, bitbucket lub cokolwiek innego.

Github szczegolnie w ostatnim czasie dodal bardzo duzo narzedzi do PR - ale to temat rzeka wiec w ogole nie bede sie na ten temat wypowiadal.

Jak wyglada moje srodowisko pracy

Od kilku lat srodkowisko w jakim pracuje wyglada wlasciwie identycznie wiec opowiem o tym jak to teraz wyglada u mnie ;) Jest tu dosc duzo rzeczy wiec postaram sie je pogrupowac.

Pracuje teraz nad dosc duza aplikacja React (~500k linii kodu nie liczac bibliotek zewnetrznych). Nad ta sama aplikacja w tym momencie pracuje mniej wiecej 30 programistow ;)

Sam proces sprawdzenia PR traktujemy tylko i wylacznie jako SANITY CHECK - ze wzgledu na ilosc standardow ktore mamy dzieki automatyzacji. Jesli jest jakas decyzja architektowaniczna to dzieje sie to poza PR - chyba, ze ktos stworzyl proof of concept ;)

Nie toleruje bycia dupkiem w komentarzach ;) Nie ma tez w PR zadnych glupich dyskusji - czasami sie zdarza ale bardzo rzadko - takie rzeczy sa zazwyczaj przegadane offline / retro ;)

• jedno repo, ~30 programistow rozbitych na 5-7 zespolow (nie wiem do konca ile aktualnie jest zespolow)
• uzywamy Jira + Bitbucket (to narzedzia od tego samego producenta wiec dobrze sie intergruja)
• starrami sie tworzyc male tickety, zazwyczaj 1 PR = 1 ticket
• nazwa branchu to nazwa ticketu + opis, np JIRA-123-make-that-button-fucking-pop
• mamy CI ale nie mamy jeszcze CD po kazdym zmergowanym PR (ze wzgledow biznesowych nie technologicznych)
• testy unit / integration / e2e
• robimy PR review dopiero jak PR przeszedl cale pipeline
• w pipeline wykonujemy wszystko co mamy zautomatyzowane (okolo 30 minut calosc trwa), lint, npm security checks, unit/int/e2e testy, sprawdzamy czy coverage nie spadl ;)
• mergujemy PR po tym jak masz 2 approval + QA approval
• mamy ludzi od QA - ale oni robia sanity check juz po tym jak PR jest review
• jesli sa jakiekolwiek zmiany (bo QA znajdzie problem), prosimy ludzi co zrobili review zeby szybko spojrzeli jeszcze na PR

Jak wyglada zycie kodu od napisania do zmergowania PR

Zalozmy, ze nasz drogi product owner chce zrobic X. Tworzymy ticket, dajemy mu jakis rozmiar (punkty czy t-shirt size).

Kazdy zespol w ktorym ja pracowalem, mial wlasne:

• ‘ways of working’ - umowa w zespole jak pracujmey
• ‘definiton of done’ - umowa w zespole kiedy mozemy uznac, ze cos jest skonczone

Dla przykladu, czescia umowy ‘ways of working’ jest to, ze kazdy kod ktory piszesz musi miec unit testy + int/e2e w zaleznosci od skomplikowania/waznosci.

Wiec, robiac X, piszemy do tego testy. Zanim kod opusci Twoj komputer, calosc musi przejsc przez linta/inne automatyczne sprawdzace co mamy - a w swiecie JS mamy ich duzo ;)

Mamy np, zdefiniowane, ze uzywamy 2 spacji etc etc.. Wszystkie te sprawdzenia sa automatyczne wiec zanim otworzyc PR zakladamy, ze wszystko bedzie juz napisanie zgodnie z syntaxem/stylem jaki sobie ustaililismy jako zespol (i robimy w tym zmiany - standardy sie zmieniaja) ;)

Poniewaz, zautomatyzowalismy te rzeczy, dyskutujemy o nich tylko jak cos chcemy zmienic - a nie przy kazdym PR ;)

Kodzik napisamy, testy napisane. Nasz CI (Jenkins w tym przypadku) wykonuje cale pipeline czy kazdym commitie w branchu - nawet jesli nie stworzyles jeszcze PR.

Czas otworzyc PR, zazwyczaj widzisz juz/zaraz bedziesz widziec status wszystkich sprawdzen (reportujemy je jako jeden check) - albo red albo green.

Jak wszystko jest zielone czas dodac kolegow z zespolu (bo pracujemy nad ta sama czescia aplikacji wiec maja context - badz ludzi z innych zespolow kiedy robimy cos wiekszego) jako reviewers i przesunac ticket do kolumny review. Jak nikt nie spojrzy na to w ciagu doby to sie upominamy zeby spojrzyli ;)

Jesli ticket, nad ktorym pracujesz jest duzy - zazwyczaj robimy kilka PR - im mniej plikow do sprawdzanie tym lepiej.

Sam proces review traktujemy tylko i wylacznie jako sanity check i nie trwa bardzo dlugo - jesli jest jakas decyzja techniczna do podjecia to przegadujemy to na etapie implementacji ;)

Dodajemy sugestie itd. Sytuacja bardzo rzadka jest, ze PR jest wycofywany (zamniety/odrzucony) bo cos jest bardzo nie-halo - i nigdy sie to nie dzieje bez dyskusji i przez osobe trzecia - ie. jesli to moj PR jest nie-halo to ja go zamkne/odrzuce a nie ktos inny nie zamieniajac ze mna slowa wczesniej.

Po tym jak PR jest sprawdzony, idzie do QA. QA sprawdza wszystkie najstarsze przegladarki jakie wlasnie wygrzebie :P Po sprawdzeniu przez QA - robimy merge - my akurat uzywamy squash (wszystkie commity na PR sa squash do jednego commita) - dzieki temu historia na masterze to historia PR. BTW. pushowanie do mastera jest zablokowane ;)

Ze wzgledu na procesy biznesowe nie robimy release po kazdym zmergowanym PR - ale tak czy inaczej jest release do produ codziennie - nawet w piatki - chyba, ze wzgledu na proces biznesowy z czym czekamy - a to tak czy inaczej max kilka dni. Produkt jest uzywamy w retail wiec okres swiat jest swiety! Wokol swiat nie ma zadnych releases - np caly grudzien nic - chyba ze naprawiamy jakis fuck-up.

Dlaczego nawet jak pracujesz sam warto tworzyc PR

Zdarzylo mi sie pare razy, ze przez jakis czas bylem jedym programista na projekcie. Tak czy inaczej dalej tworzylem PR, glownie ze wzgledu na CI - mimo, ze sam sprawdzalem sobie PR (a czasami cos zauwazylem i poprawilem) to cale flow bylo oparte o PR. Glowne zalozenie jest zawsze takie, ze master jest od razu wrzucany na produkcje lub zawsze jest gotowy do wypuszczenia. Wiec, ja moge sobie spokojnie pracowac na branchu, stworzyc PR kiedy skoncze jakas funkcjonalnosc, wszystkie testy przeszly wiec - merge it!

Nawet piszac to stworzylem PR, mimo, ze bedzie to jeden commit (chyba, ze znajde blad ;)).

I to by bylo na tyle… mam nadzieje, ze bylo to choc troche ciekawe. Kazdy z tych tematow to temat rzeka. Moze zrobic z tego serie? :) Pozyjemy zobaczymy ;)

Yo! Piotr Bogusław Seefeld