Lobiチームの長田です。
今回はLobiチームのサーバーサイドエンジニア間で採用されているレビューのルールについて紹介します。 なお、ここで言う「レビュー」とは一般的には「コードレビュー」と言われているものです。 Lobiチーム内では「レビュー」のひとつのステップとして「コードレビュー」が存在するため、 このエントリでは「レビュー」と「コードレビュー」を書き分けることにします。
レビューの目的
レビューは以下の目的で行なっています。
- 不具合・障害の原因が含まれた変更が本番環境に反映されることを防ぐ(品質向上)
- 変更内容をチームメンバーに共有する(属人化防止)
- レビュワーが変更内容を見て知識・テクニックを吸収する(教育)
これらを達成するために、変更内容に全く関係しないメンバーへのレビュー依頼も行われています。
環境
Lobiチームではソースコードをgitで管理し、GitHubにホスティングしています。 レビューはGithubのPull Request上で行なっています。
gitのブランチ管理はGitHub Flowに準拠しています。 レビューは対象がmasterブランチである場合に限らず、マージが発生する際には必須としています。
o---o topic
/ \ <-----------------review
o---o---o---o---o topic master
/ \ <-----------review
---o---o---o---o---o---o---o master
コードレビュー
コードレビューはPull Requestが作られると必ず行われるレビューです。 変更内容全般についてその内容で問題ないか、本番反映手順に不備はないかがチェックされます。
レビュー依頼はサーバーサイドエンジニアであればだれにでも行うことができるようになっています。 「レビューの目的」でも書いたように、変更内容の共有などの目的もあるため、 その変更に全く関わっていないメンバーであってもレビュー依頼は受けなければなりません。
そうはいってもレビュー内容に全く自信がない！という場合は 自分もレビューした上で 別のメンバーにレビューを再依頼することができることにしています。
また、指名されていないメンバーが勝手にレビューすることも推奨しています。
(特に最近はGo言語で書かれたPull Requestが作られるとGoおじさんたちがこぞってマサカリ
ひとつのPull Rquestに8人(ひとりは後述のnuko bot)でやんややんや言っているの図↓
クエリレビュー
クエリレビューは特にデータベースへの操作について重点的にレビューを行うものです。 データベースは特にスケーリングが難しくなおかつ負荷増加がサービス全体に影響する可能性が高いので、 コードレビューとは別にこのレビューを行なっています。
クエリに変更がない(と作業者が思っている)Pull Requestであっても、 「クエリに変更がない」ことを確認する目的でクエリレビューが行われます。
クエリレビューはデータベースの知識を持った特定のメンバーだけが行います。
インフラレビュー
Lobiではプロビジョニングツールとしてchefを使用していますが、 これについて変更を加えた場合はインフラレビューを通過する必要があります。
インフラレビューはchefによる影響範囲を把握しているインフラ担当者だけが行います。
その他
変更が大きい場合
レビューをするにはそれなりの集中力が必要になるので、変更内容が多くなるほどレビュー精度低下します。 レビュー精度が低下すれば品質向上の役割を果たせなくなってしまいます。 また、差分が多い変更はレビューによる戻しが多くなる傾向があるので、その対応を行う作業者の負担も増加することになります。 (5回目の戻し、とか、お互い何をはなしているのかわからなくなってきますからね！)
変更内容が一定量以上の場合、具体的にはdiffが200行以上ある場合は二人以上のコードレビューを通過するルールにしています。 つまり、差分が200行以上あるPull Requestをマージするためには、
- コードレビュー
- コードレビュー二人目
- クエリレビュー
の計3回のレビューを通過する必要があります。
追加レビュー
「コードレビュー」の項でも触れましたが、変更が少ない場合でも、
- レビュー依頼者がセカンドオピニオンを求めている場合
- レビュワーが自分のレビューだけでは不安を感じる場合
などは二人目のレビュワーを指名しても良いというルールにしています。 不安があるのであればそれを具体的な問題点として挙げられるようにするべきですし、 レビュワーの立場からしても同じコードを見たときに他人がどのような指摘を行うのかは興味があるところです。
レビューのコスト
レビューにはそれなりの時間がかかります。 最低2回、多い場合には3回以上のレビューを通過しなければならないということは、 レビュワーの時間が大きく削られるということです。
しかし、レビューの目的としている
- 品質向上
- 属人化防止
- 教育
についてはそのコストを払うに値する効果が得られていると判断しています。 不具合がそのまま本番環境にデプロイされた場合に対応コストを払った上で最悪ユーザーにまで影響が出る可能性を考えると、 「属人化防止」「教育」もまとめてできるレビューはむしろコストパフォーマンスが良いと言えるかもしれません。
nuko
レビュー依頼は、レビュワーの時間を奪ってしまうことから少々気が引ける行為です。 Lobiチームではこの罪悪感(？)を軽減するためにレビューガチャという仕組みがあります。
Slackにnuko
というbotが常駐しており、このbotに話しかけるとレビュワーをランダムに割り当ててくれます。
(#のあとにはPull Requestの番号が続きます)
ランダムとは言っても直近にレビュワーとして指名されている場合は対象から外れたり、 レビュー依頼を投げた本人には当たらないなどの考慮はされるようにしています。
レビューが終わったら、変更をマージしても問題ないと判断した場合は「lgtm」
イマイチだと判断した場合は「back」と書けば
Pull RequestのAsigneeとLabelが更新されるようになっています。 なにぶんレビューの種類が3種類もあり関連するLabelの種類も多いので、自動付け外し機能は重宝しています。
nukoはGoで書かれているので、Goの勉強がてら機能を拡張するということも行われているようです。
コード以外も
ちなみにこのエントリも公開前にPull Requsetとしてチームメンバーに投げてレビューしてもらっています。
おわり
今年の1月から続けてきたLobi紹介連載ですが、今回でひとまず終了となります。 紹介しきれていないトピックはまだ残っているので、今後は不定期に紹介エントリを上げていこうと思います。 (毎月締め切りがあるのはつらい！)
現在アドベントカレンダーも実施中なので、ぜひそちらも覗いてみて下さい。
