1. Teoria - audyt vs review

Updated 2,331 Days AgoPublic

Actions

Audyt vs Review

Phabricator (program napędziający Sealhuba) zawiera dwie duże funkcjonalności umożliwiające wzajemne "przeglądanie kodu" przez pracujących nad nim programistów: Audyty i Review. O ile mogą się one zdawać na pierwszy rzut oka funkcjonalnie tożsame, każda z nich ma swoje unikalne cechy, które sprawiają, że pasuje ona lepiej do niektórych scenariuszy, niż ta druga.

W telegraficznym skrócie:

Warto też przeczytać, co na temat tego rozróżnienia napisali twórcy Phabricatora: https://secure.phabricator.com/book/phabricator/article/reviews_vs_audit/

Audyt

Audyt tyczy się pojedynczego commita i zachodzi po wysłaniu commita na repozytorium. Dany commit może mieć stan "wymaga audytu" lub "nie wymaga audytu".

Powyższy zrzut ekranu przedstawia część widoku repozytorium DlaSchroniska w webowej przeglądarce repozytoriów. Widzimy na nim trzy commmity, (po kolei, od góry):

• pierwszy z nich nigdy nie wymagał audytu,
• drugi z nich wymagał audytu i dostał pozytywną opinię (czyli już nie wymaga audytu),
• trzeci z nich wymagał audytu i ktoś zgłosił pewne obawy, którymi należy się zająć (czyli wciąż proces audytu nie jest zakończony).

Jeżeli ktoś zgłosi w trakcie audytu potrzebę zmian, należy stworzyć i spuszować osobny commit, który poprawia te zmiany.

Aby sprawić, że commit wymaga audytu, należy dodać osobę do listy audytorów.

W trakcie audytu można nanosić komentarze "inline" do zmian z danego commita, ale nie można wprowadzać już zmian w wysłanym commicie, można tylko wysłać nowy commit, który naprawia coś z danego commita. Jest to istotna różnica w porównaniu z Review.

Za pomocą narzędzia Herald można stworzyć reguły, które automatycznie dopisują kogoś do listy audytorów, jeżeli dany commit spełnia pewne konfigurowalne warunki (np. "Dodawaj Arkadiusza do listy audytorów commitów, które zmieniają coś w CSS.").

Tutaj informacja o tym, jak wygląda Audyt z praktycznej strony

Review

(będe używał słów "Review" i "recenzja" zamiennie, z powodów stylistycznych)

Review tyczy się większych zmian, wymagających konsultacji z innymi członkami zespołu. Review zachodzi przed wysłaniem zmian na repozytorium. Prosty przykładowy flow prezentuje się następująco:

1. Deweloper A otwiera feature-branch na lokalnym repozytorium;
2. Deweloper A tworzy kilka commitów;
3. Deweloper A decyduje, że nowa funkcjonalność jest gotowa i chce poddać ją review. Tworzy więc diffa swojego feature-brancha z masterem i umieszcza go w Differential, np. korzystając z interfejsu webowego;
4. Deweloperzy B i C umieszczają komentarze w diffie i sugerują jakieś zmiany;
5. Deweloper A nanosi swoje zmiany w lokalnym repozytorium, tworzy nowego diffa i wkleja go w interfejsie webowym jako nową wersję poprzedniego diffa (a nie jako nowego diffa); //kroki 4 i 5 powtarzają się tak długo, aż zespół jednogłośnie akceptuje diffa
6. Następuje tzw. "land". Deweloper A squashuje swoje commity z brancha w jeden i dokleja go jako pojedynczy commit w defaultowym branchu, a następnie pushuje go.

(Oczywiście nikt nie musi ręcnie generować diffow, przeklejać ich ręcznie z terminala do interfejsu webowego ani wiedzieć, jak się squashuje commity - istnieje narzędzie CLI, które robi to automatycznie: Arcanist. Możesz dowiedzieć się o nim więcej kilka paragrafów poniżej).

Review stanowi pewne zaprzeczenie znanemu już GitHub Workflow, gdyż flow zamiast tak:

write -> publish -> review -> merge

wygląda tak:

write -> review -> merge -> publish

Zalety płynące z Review to, m.in:

• na repozytorium nie lądują nieprzejrzane commity - zmniejsza się drastycznie prawdopodobieństwo potrzeby revertów lub commitów naprawiających;
• zachęca autorów do tworzenia możliwie małych, zrozumiałych zmian;
• daje recenzentom okazję do sugerowania istotnych zmian w kodzie, co jest trudne przy np. robieniu audytu commita, który wylądował na repozytorium 15 commitów temu;
• autor diffa może poprosić recenzentów o wprowadzenie poprawek;
• zespół jest wdrożony w nowe zmiany w projekcie - na produkcji nie ląduje kod, którego wcześniej nie widzieli;
• motywuje autora diffa to reagowania na zmiany - ponieważ review blokuje zmiany z negatywną recenzją;
• sprawia, że historia commitów wygląda czytelniej i ma bardziej płaską strukturę - każdy commit tyczy się pojedynczej, izolowanej zmiany i ma tytuł niosący za sobą znaczenie (Arcanist pozwala na automatyczne generowanie tytułów commitów na podstawie danych z Review). Przykład repozytorium rozwijanego wg Review Workflow:

dlaschro/ $ git log --graph --oneline
* e59b4f4 Podmień zdjęcie Anny Lucińskiej ref T284
* e2cdfae Dodaj dwóch nowych partnerów
* c66418e Usuń baner akcji 'Smycz dla pisiaka'
* c625894 Usunięcie niedozwolonych statusów w panelu darczyńcy
* 870f96f Wyrównaj kolumny w widoku edycji potrzeb
* 8fe56c4 Dodaj możliwość zmiany stanu zamówienia z 'anulowane' na 'oczekuje na płatność'
* 7c21b80 Popraw niepojawiający się przycisk 'czytaj więcej' w Chrome
* 83f04cf Dodaj czyszczenie cache'u HTTP po utworzeniu zwierzaka
...

Przykład z repozytorium rozwijanego wg Github Workflow:

rails/ $ git log --oneline --graph
* b635eea use kwargs to avoid hash slicing
*   8c53b41 Merge pull request #23611 from abhishekjain16/routes_options
|\  
| * 4e3931a Fixes routes to match verbs and path with -g option
* | da1fbb9 Add fixes accidentally removed.
* | 354fb73 Flesh out request encoding + response parsing changelog entry.
* |   d50d709 Merge pull request #23642 from tvon/chore/fix-path-omission
|\ \  
| * | 1a61496 Use correct path in documentation.
* | |   db64cba Merge pull request #23641 from abhishekjain16/docs_fix
|\ \ \  
| |/ /  
|/| |   
| * | 9a2ca9c [ci skip] Fix enqueuing spelling to maintain consistency
|/ /  
* |   c63f58d Merge pull request #23639 from Gaurav2728/update_change_log_entry_action_pack
|\ \  
| * | 33e202d use rails instead of rake
|/ /  
...
| |/ / / / / / / /  
|/| | | | | | | |   
| * | | | | | | | 03980b2 Converting backtrace to strings before calling set_backtrace
* | | | | | | | |   2db347b Merge pull request #23395 from PareshGupta/remove-unused-constant
|\ \ \ \ \ \ \ \ \  
...

Zachęcam do lektury: https://secure.phabricator.com/phame/post/view/766/write_review_merge_publish_phabricator_review_workflow/

Oraz do zapoznania się z instruktarzem krok-po-kroku dot. Review

Kiedy korzystać z Review, a kiedy z Audytów?

Mając na uwadze plusy i minusy tych rozwiązań zależnie od scenariusza w projektach stosujemy obydwa podejścia - zależnie od charakteru danego taska. Część tasków wymaga solidnych dyskusji i konsultacji z zespołem, a część wymaga nagłego działania i prostych zmian w kodzie. Nowe widoki, zmiany w API itp bardziej proszą się o Review, a szybkie fixy o proste audyty.

Uznajemy "Review" za domyślny sposób na dodawanie zmian w repozytoriach, nad którymi pracuje kilka lub więcej osób. Tryb Review można pominąć na rzecz Audytu w szczególnych przypadkach, zwłaszcza gdy:

• zmiana jest jednolinijkowcem i naprawia oczywisty błąd programistyczny (błąd składni, literówka, zły url, itp)
• zmiana jest krótka, i nie wpływa na API ani na strukturę kodu - np. dodanie nowego elementu do jakiejś tablicy, podmiana zdjęcia/logo (poparta powodem - np. życzenie klienta / zleceniodawcy).

Dla ułatwienia komunikacji można oznaczać zadania tagami "Tryb Review" i "Tryb Audytu".

Do danego zespołu projektowego należy decyzja o tym, kto i jak decyduje, czy dany task zasługuje na Review czy na Audyt. Generalnie są dwie główne opcje:

• decyzja należy do osoby wykonującej daną zmianę w projekcie - zalecane dla zespołu doświadczonego w odróżnianiu Audytów od Review
• zespół podejmuje wspólnie decyzję odnośnie tego, czy dany task jest Audytowany czy Reviewowany. Zaleca się wtedy oznaczenie taska odpowiednimi tagami.