Z pamiętnika Confitury 2013 - recenzja Review your code like a Googler Darka Łuksza

Pełne "wgryzienie się" w występ Darka Łuksza zajęło mi prawie tydzień. Każdego dnia przez ostatni tydzień udawało mi się wydzielić kawałek czasu, aby wsłuchać się w prezentację na temat "Review your code like a Googler", w którym +Dariusz Łuksza przedstawiał Gerrita i jego integrację z Eclipse IDE, a z nimi powody, dla których warto rozważyć wdrożenie codziennych przeglądów kodu w Twoim zespole.

Poniżej co ważniejsze kawałki z prezentacji. Jest ich całkiem sporo, co w zasadzie udowadnia bogactwo merytoryczne prezentacji. I taka faktycznie była! Chyba się z nią zżyłem po tym prawie tygodniu oglądania i styl prezentacji Darka zaprocentuje w przyszłości wdrożeniem gerrita u mnie w zespole. Citi pracuje na CollabNet TeamForge, a Darek właśnie integruje gerrita z TeamForge w CollabNet, więc pewnie już niedługo będę mógł powiedzieć coś więcej w temacie praktycznie.

Przed każdym cytatem (wybacz Darku, jeśli nie w pełni oddałem sens wypowiedzi lub zmieniłem tu i ówdzie) jest czas - w formacie minuta:sekunda - kiedy dany kawałek się pojawi. Przy niektórych pojawia się również mój komentarz.

Niektóre z cytatów można umieścić śmiało w sygnaturze maila (!) Zdecydowanie polecam tę prezentację. Dzięki Darku!

01:06 "Tak więc zakładam, że znacie gita. Wiecie, co to jest push, pull, fetch, merge, rebase, refspec." - Akurat w moim przypadku rebase i refspec nie są znane. Do nadrobienia.

01:20 "Prezentacja nie jest dla osób, które się wstydzą swojego kodu. Wasz kod nie jest Waszą żoną. Możecie się nim dzielić. I oto chodzi."

01:29 "Dzięki temu, że na kod patrzy więcej osób, wyłapywanych jest więcej błędów. Tak samo Wy się uczycie. Uczycie też innych."

02:09 "Dzisiaj, wyjątkowo, będę używał do współpracy z Gerritem i Gitem Eclipse z Egit'em oraz Mylyn Connector. Jeżeli interesuje Was użycie z 'command line', zapraszam na prezentację Mateusza Szczap dzisiaj w Marmoladzie. Tam będzie więcej na temat, jak można jeszcze bardziej zautomatyzować proces 'code review' korzystając z komand ssh, które udostępnia Gerrit."

03:00 Wzdychając..."Ok, to 'code review'. O 'code review' słyszałem kilka ładnych lat temu."...kładzie rękę na policzku, jakby cierpiał z bólu zęba, a to tylko 'code review' :-)

03:27 "Po co robimy 'code review'? Robimy 'code review' po to, żeby minimalizować liczbę błędów. Żeby te błędy wyłapywać wtedy, kiedy one są bardzo tanie i proste do poprawienia, czyli wtedy, kiedy kod jest tworzony, kiedy dany 'feature' jest tworzony, a nie kiedy aplikacja znajdzie się na produkcji i przyjdzie 100 niezadowolonych użytkowników (o których była mowa na poprzedniej prezentacji) i powie że coś nie działa. Wtedy naprawienie tego błędu jest bardzo kosztowne i też bardzo stresujące dla nas, developerów, że coś takiego przeoczyliśmy i zapomnieliśmy o czymś takim wspomnieć."

04:00 "'code review' robimy też po to, żeby dzielić się naszą wiedzą i żeby się uczyć."

04:09 "Są osoby w zespole, które poświęcają pewien czas, żeby się dokształcać. Po to też 'code review' się robi, żeby te osoby mogły swoją wiedzę dzielić wśród zespołu, żeby ta nasza aplikacja, to nasze dziecko, nasze dzieło, które robimy było lepsze."

04:25 "Pierwszy typ 'code review' - 'post-commit review', tzw. 'formal code review'."

04:38 "Tą słynną metodologię liczenia jakości kodu przez WTF/minute. Dzisiaj zrobimy coś więcej."

05:00 "Bardzo ważnym elementem na tym zdjęciu jest...kubek z kawą, a właściwie termos." Auć! Ja kawy nie piję! Czy to miałoby oznaczać, że nie dla mnie taki zespół?! :-)

05:22 "Co po tym będzie? Potem będzie dokumentacja. Potem będzie 'bug' w 'bug trackerze', a potem będzie 'backlog'. Z mojego doświadczenia wynika, że takie błędy nie są poprawiane, o ile to nie jest to błąd bardzo duży i kosztowny, który łamie jakieś założenia aplikacji, który jest w logice biznesowej."

05:46 "W tym momencie ten kod już jest w repozytorium. Ktoś inny bazuje na tym kodzie. Co więcej ta osoba może bazować na zachowaniach tego kodu, które są błędne."

06:22 "Kolejny typ ['code review'] 'review on request'."

07:48 "Na końcu to, co ja nazywam asynchronicznym 'code review'. To, co jest możliwe do osiągnięcia z gerritem i to, co moim zdaniem jest najlepsze z możliwości, które nam oferuje aktualnie rynek."

08:45 "Teraz 'post-commit code review' vs 'pre-commit code review'."

08:55 "Reprezentacja graficzna? Jeżeli robicie 'code review' po 'commicie', kończycie z wielką toną dokumentacji i rzeczy, które zrobimy kiedyś, czyli nigdy. Jeżeli macie 'code review' przez 'committem', to macie większe ciśnienie, żeby to 'code review' zrobić, bo 'feature' nie wejdzie do aplikacji, bo klient nie będzie zadowolony, bo szef nie będzie zadowolony, bo ja nie skończę mojego zadania, bo ktoś nie zrobił mi 'code review', bo ja nie zrobiłem komuś 'code review'. Chodzi o to, żeby w tym wypadku,  musimy się do tego ['code review'] jakoś zmusić."

09:27 "To podejście ['pre-commit code review' z gerrit] daje takie narzędzie, które po prostu zmusza. Jeżeli wy nie będziecie robić 'code review', to aplikacja będzie stała w miejscu."

09:40 "Przejdźmy do gerrita."

09:42 "Czym jest gerrit? I dlaczego taki tytuł prezentacji?"

09:46 "Gerrit powstał tylko i wyłącznie dla projektu Android po to, żeby przenieść 'feeling', który jest wewnątrz Google z ich 'code review' na społeczność 'open source', na projekt Android."

10:00 "Cytat, który był na samym początku, że "żadna zmiana nie wchodzi bez 'review' do repozytorium", to jest jedna z zasad, która obowiązuje w Google. Tam każdy kod przechodzi przez 'review'."

10:35 "Gerrit jest bardzo silnie spięty z gitem. Bazuje na gicie. Możecie używać gerrita, tak jakby to był serwer gita. Nie musicie używać 'code review' związanego z gerritem. Możecie w gerricie, tak go skonfigurować, że mając 30 repozytoriów, 3 z nich będą miały 'code review', a pozostałe będzie można 'pushować' do serwera gita normalnego. Co więcej, macie możliwość ustawienia 'access controls' per 'branch', per 'branch', gdzie 'branch name' może być wyrażeniem regularnym. To nie są takie proste ustawienia ACLe, jak np. odczyt i zapis. To jest 'pushowanie merdży', 'puszowanie tagów', zmiana autora 'commitu', czyli 'puszowanie' jak ktoś inny. Tych ustawień jest sporo i ta lista przyrasta."

11:30 "Jeśli nie potrzebujecie korzystać z 'code review', a potrzebujecie serwera gita, to polecam Wam właśnie gerrita" - cóż za rym! - "Aplikacja javowa, webowa, czyli mamy znaną nam architekturę - prostokąt, prostokąt, cylinder, tj. frontend, backend, na frontendzie jest GWT, na backendzie jest Guice plus PostgreSQL."

11:56 "Dość lekkie [gerrit], zwinne. Nie potrzebujecie żadnego kontenera. Odpalacie to normalną komendą java -jar i podajecie ścieżkę do jara gerrita. Koniec. Nawet do testów bardzo fajne."

12:46 "Podejrzewam, że ten projekt [gerrit] może przeżyć samego Androida, dlatego że jest wykorzystywany do większej ilości zadań niż tylko jako system bazowy."

13:00 "Pobawmy się trochę gerritem!"

13:50 "Fred i jego IDE - oczywiście Eclipse. I co Fred chce zrobić? Fred jako, że jest nowy chce zrobić pierwszą zmianę. Niech to będzie zmiana, która będzie coś psuła." - po prostu miodzio mieć takiego nowego w zespole, co?

14:03 "Oczywiście, jak to w git'cie, zakładamy nowego 'brancha' na samym początku."

14:16 "Popsujmy sobie 'builda'."

14:20 "Każdy z nas wie, jak się najprościej psuje 'builda' - psuje się testy jednostkowe, które oczywiście mamy w aplikacji."

15:00 "Bardzo ważna rzecz. Tutaj jest taki bardzo enigmatyczny ciąg znaków - Change-Id: i same zera. Tak to musi być na początku. To jest generowane po 'commit'cie', zostanie zastąpione pewnym specjalnym numerkiem, 'hashem'. Po tej linii, Gerrit rozpoznaje 'commity'. Dzięki tej linii, on jest w stanie spiąć kolejną wersję Waszego 'patcha' z całym 'code review'."

15:36 "Możecie zmienić cały 'commit message'. Możecie zmienić dowolnie 'content', który jest w środku, ale ważne, aby ta jedna linia [z Change-Id] została niezmieniona. Teraz tam są same zera, bo zostanie zaraz wygenerowana."

16:16 "'Pushujemy' 'Push to Gerrit'. Nie bezpośrednio, tylko po prostu wybieramy tę opcję z menu."

16:25 "'Pushujemy' do specjalnego 'brancha', który się nazywa refs/for/. W tym wypadku jest refs/for/ 'master'. Podajemy docelowy 'branch', do którego ta zmiana ma trafić po samym 'code review'. Jeśli ta zmiana ma trafić do 'brancha stable' podajemy refs/for/ 'stable' zamiast 'master'."

17:44 "To, co jest fajne z gerrittem to, to, że można go bardzo łatwo spiąć z Jenkinsem. W tym momencie, po każdym 'pushu for review' odpalany jest 'job' na Jenkinsie, ewentualnie Hudsonie. 'Job' jest wykonywany i możecie go dowolnie skonfigurować - w tym wypadku po prostu odpala testy. Jeżeli zmiana się nie skompiluje, albo 'sfailują' testy, Jenkins nam taką zmianę zablokuje. W tym wypadku głosuje 'Code Review' -1, bo się posypały testy. Już w tym momencie nie musicie patrzeć na testy, patrzeć w 'diffa', widzicie to aktualnie, że ta zmiana coś psuje - nie jest gotowa, więc pomijacie ją. Omijacie jeden element z Waszego dziennego planu zadań, czyli 'code review'."

18:16 Dlaczego nagranie przechodzi w tryb dwóch paneli, gdzie lewy jest pusty?! Bardzo utrudnia oglądanie nagrania.

18:40 "Poprawmy tę zmianę...Tak samo jak wcześniej 'scommituję' używając skrótu i w tym momencie zaznaczam 'amend'."

+Dariusz Łuksza - dlaczego amend a nie po prostu kolejny commit? W końcu to jest już w historii, chociażby Jenkinsa, że coś się sypnęło i stąd kolejna zmiana.

Niestety, w pewnym momencie prezentacja gerrita zamienia się w prezentację integracji Eclipse IDE z gerritem.

21:54 "Trzeba zrobić 'push comments'. W tym momencie macie również możliwość głosowania na zmianę. Ten gerrit, który mam lokalnie skonfigurowany w ten sposób, że na 'verify' głosuje tylko i wyłącznie Jenkins. Nikt inny. Odsiewam te zmiany, które się automatycznie nie zbudują, albo nie przejdą testy. Dla mnie ważny jest tylko 'code review', czyli to, co jest w kodzie."

22:20 "Zakres, jaki mamy możliwy do zrobienia tutaj, to jest od -2 do 2. To są flagi. One nie sumują się w żaden sposób. Flaga -2 to jest jedna flaga, która jest przyklejona. Oznacza, że ta zmiana nigdy nie powinna wejść do repozytorium...Ona jest przyklejona w ten sposób, że po 'spushowaniu' kolejnego 'patchseta', który będzie coś poprawiał, -2 zostaje. W pozostałych przypadkach wszystko jest 'resetowane' do zera. -1 - drobne uchybienia w kodzie...+1 - wygląda dla mnie dobrze, ale wolę, aby ktoś jeszcze na to spojrzał...+2 - jest dla mnie ok, może iść do repozytorium. Dlaczego jest takie rozróżnienie pomiędzy +2 a +1? Dlatego, że możecie nie mieć dostępu do +2. Do każdego takiego poziomu, do każdej flagi można ustawić dostęp. Dana grupa będzie mogła tylko taką wagę ustawić."

23:38 "Co do grup - mechanizm jest dość skomplikowany, bo grupa może 'includować' grupę, może być oddelegowana, np. do LDAP. Grupa może być właścicielem innej grupy, więc poziom skomplikowania konfiguracji ACLi w gerricie jest dość wysoki."

24:35 "Zmianę można porzucić. Po prostu klikamy przycisk 'Abandon', podajemy odpowiedni komentarz, dlaczego to porzucamy. Ta zmiana zostaje w gerricie. Można ją potem wskrzesić. Ogólnie, nic z gerrita nie znika. Wszystkie komentarze, które zrobicie w gerricie i je zapiszecie są w gerricie. Wszystkie zmiany, 'patchsety', które 'pushujecie' są w gerricie. Tak więc możecie je potem odzyskać."

25:12 "Zróbmy coś bardziej skomplikowanego...Przede wszystkim zaakceptujmy jakąś zmianę, a po drugie, te zmiany będą ze sobą połączone."

28:24 "Po to się robi 'branche' tematyczne, żeby rozłączne zmiany trzymać osobno. Jeżeli zmiany między sobą zależą, 'pushujecie' je po kolei, tak jak pokazałem. Nie ważne, czy są dwa 'commity' czy 80 'commitów', 'pushujecie' je razem. Macie zapewnione przez gerrita, że 'commit' 77 wejdzie po 76."

// w tym momencie pada pytanie z sali

29:21 "Właśnie! Konflikty! Dobre pytanie."

29:28 "Z konfliktami jest w ten sposób, że gerrit część potrafi konfliktów sam rozwiązać. On używa jgita, który ma algorytm do rozwiązywania konfliktów, ale jeżeli ich nie potrafi rozwiązać, po prostu powie "sorry Winetou, ktoś tu musi przyjść i 'zmergeować'"".

30:00 ekran wraca do normalnych rozmiarów.

34:20 Koniec przykładów.

34:40 "Parę 'hintów' odnośnie 'code review'." => na slajdzie pojawiły się punkty: 'It's like TDD, review code before it hits main repository.' oraz 'Do code review on daily basis...remember when you are not reviewing other's code then application's not progressing.'

34:42 "Przede wszystkim [...] bądźcie konstruktywni i nie piszcie takich rzeczy typu 'Taki kod ja pisałem w liceum', albo 'To nigdy nie powinno wejść' bez podania dlaczego. Piszecie to tak, jakbyście pisali komentarz do kodu, a nie do osoby. Starajcie się ten kod przeglądać tak, jakby to był Wasz kod. Wasz kod, który napisaliście pół roku wcześniej, wracacie i myślicie 'Co ja tu miałem na myśli?! Gdzie ja tu mogę coś zrefaktorować? Gdzie mogę coś poprawić? Gdzie użyć nowej biblioteki, o której dowiedziałem się tydzień temu?'"

35:20 "Jeżeli dostajecie 'code review', to nie jest tak, że ktoś mówi Wam 'Jesteś głupi. Nie widziałeś tego NullPointerException?!' a raczej 'W tym kodzie jest błąd i to powinieneś poprawić. Następnym razem po prostu uważaj na to, że ta metoda zwraca nulla. Mogłeś o tym nie wiedzieć.' Chodzi o to, żeby sobie pomagać. Żeby razem to nasze małe zwierzątko zwane aplikacją wychować tak, żeby zachowywało się dobrze na produkcji."

35:46 "Code review powinno być robione codziennie."