組込みの設計レビューをGitHubのPRに寄せる

組込みCの設計レビューをGitHubのPRに寄せてみる

組込みCのリポジトリで、会議室でやっていた設計レビューを、Pull Request（以下PR）の上でやるようにした。

CODEOWNERSやブランチ保護の設定そのものより、今までレビューでやっていたことをどう移すか、という話になる。

この記事中のチーム名、パス、ルールの細かい数値などは一般化している。構成と考え方は実運用のものだが、そのままコピペして動くものではないのであしからず。

なお、GitHubの標準機能で済むところと、こちらで決めた運用が混ざるので、節ごとに [標準機能] と [今回の工夫] を付けておく。設定を真似する前に、どこまでがGitHubの保証で、どこからが自分たちのローカルルールかは分かるようにしておきたい。

前提

今までの設計レビューは、だいたいこうなっていた。

• レビュー用の資料を作る
• 日程を調整する
• 集まって説明して、指摘をもらう
• 指摘と対応状況を別の一覧で管理する

変更が大きいときはこれでいいのだが、小さい修正でも同じ手順を踏むことになる。それで「まとめてレビューしよう」となり、結果として大きな塊を一度にレビューになっちゃう。

あと、指摘がコードから離れた場所に書かれるので、あとから「この指摘はどの修正で直ったのか」を追いにくい。

PRなら、差分とコメントと承認が同じ場所に残る。それならそっちに寄せたい、というのが出発点になる。

レビューでやっていたことを分ける — [今回の工夫]

「レビューをPRにする」と言っても、会議でやっていたことは1つじゃない。まず分けてみた。

会議でやっていたこと	PRでの置き換え	分類
変更内容の説明	PRの説明文と差分	標準機能
指摘を出す	行コメント、suggested changes	標準機能
見るべき人が見たかの確認	CODEOWNERS	標準機能
指摘の対応管理	conversationのresolve	標準機能
承認した記録	Approveとその履歴	標準機能
設計方針そのものの合意	（PRだけだと難しい）	運用で補う

置き換え先はほぼ標準機能で足りる。工夫があるとすれば、この表を作って「どれが移せてどれが移せないか」を先に決めたこと自体だと思う。

一番下だけは無理に移さないことにした。方針が固まっていない状態でPRを出すと、差分の上で設計議論が始まってしまう。詳しくは後ろに書く。

まず考えたこと — [今回の工夫]

PRベースにするときに、特に避けたかったのは以下。

困ること	今回の対応	分類
誰も中身を見ないまま承認される	CODEOWNERSで見る人を自動で指名する	標準機能
承認をもらったあとに直してマージする	新しいcommitで古い承認を無効にする	標準機能（既定はオフ）
指摘が流れて、対応されたか分からない	未解決のコメントがあるとマージできないようにする	標準機能（既定はオフ）
特定の人にレビューが集中する	CODEOWNERSは個人ではなくチームで書く	書き方の工夫
急ぎを理由に、ルールごと迂回される	管理者にも同じルールを適用する	標準機能＋運用判断

対応はほぼ標準機能で足りる。ただし、いくつかは既定でオフになっている。何も設定せずにPRを使い始めると、このへんは効いていない。

工夫があるとすれば、先に「避けたいこと」を出してから設定を選んだ順番のほうで、設定項目を上から眺めて決めたわけではない。

誰が見るかをファイルに書く — [標準機能]

.github/CODEOWNERSに、パスごとのレビュアーを書く。ファイル名も場所も書式もGitHubが決めているもので、置けばPR作成時に自動でレビュアーが指名される。

*                @example-org/fw-reviewers

/src/drivers/    @example-org/fw-hw-owners
/src/control/    @example-org/fw-control-owners
/include/        @example-org/fw-reviewers @example-org/fw-control-owners

/.github/        @example-org/fw-leads
/docs/design/    @example-org/fw-leads

後ろに書いたパターンが勝つので、先頭に広いもの、後ろに細かいものを置く。この優先順位もGitHubの仕様。

ここから先は書き方の話になる。

指定は個人ではなくチームにした。これは標準機能ではどちらも書けるが、個人名で書くと、その人が休んだ時点でPRが止まる。

今までは「この変更はあの人にも見てもらったほうがいい」を、主催者の記憶で判断していた。それをファイルに落とすだけでも、声を掛け忘れる事故は減ると思う。

ただ、少人数のチームで全パスにオーナーを付けると、結局同じ人が全PRに指名される。
最初は影響の大きい領域だけにして、様子を見ながら広げるくらいでよさそう。
この「どこから付けるか」は完全にこちらの判断で、ツール側は何も言ってくれない。

見ないとマージできない状態にする — [標準機能]

CODEOWNERSを置いただけでは、レビューは必須にならない。ブランチ保護（またはルールセット）で止める必要がある。

入れた設定はこのあたり。全部GitHubの設定項目で、独自の仕組みは足していない。

• PRを経由しないとマージできない
• 承認が1件以上必要
• 新しいcommitがpushされたら、前の承認を無効にする
• Code Ownersの承認を必須にする
• 未解決のコメントが残っているとマージできない
• ビルドや静的解析のチェックが通っていること
• 管理者にも同じルールを適用する

効くと思ったのは、3つめの「前の承認を無効にする」。

承認をもらったあとに中身を直してマージできてしまうと、承認の記録が何も保証しなくなる。ここが緩いと、あとから履歴を見ても意味がない。既定ではオフなので、意識して入れる必要がある。

管理者を例外にするかどうかは迷った。これは設定としてはどちらも選べるので、こちらの判断になる。

例外を作ると、たぶん「急ぎだから」で使われる。それが1回起きると、次からは普通のことになる。

なので通常のルールには穴を開けず、どうしても必要なときの手順を別に決めることにした。

説明を書く側に寄せる — [今回の工夫]

会議なら、説明しながら進められる。PRではそれがないので、その分は書いてもらうことになる。

PRテンプレート自体は標準機能で、.github/pull_request_template.mdを置けばPR作成時に本文へ入る。中身は自由なので、ここからは各チームで決める部分になる。

## 変更内容

## なぜこうしたか
（検討した別案があれば、それも）

## 影響範囲
（関連するモジュール、対象の機種や構成）

## 確認したこと
- [ ] 対象構成でビルドした
- [ ] 静的解析の結果を見た
- [ ] テストを実施した

## 見てほしいところ

「なぜこうしたか」と「見てほしいところ」は、書く側の手間が増える。

それでも残したのは、この2つがないと、レビューする側が差分から意図を推測することになるため。推測でレビューすると、指摘が的外れになりやすく、結局コメントのやり取りが増える。

チェックボックスは、GitHubとしては単なる表示で、未チェックでもマージは止まらない。本当に止めたいものはCIのチェックにすべきで、ここに書くのは「人が確認したと言っているだけ」のもの、という切り分けにしている。

差分の大きさも、テンプレートでは解決しない。「1つのPRに1つの意図」を目安にして、リファクタリングと機能追加は分けてもらうようにしている。これは完全に運用のお願いで、強制はできていない。

会議をやめない部分 — [今回の工夫]

全部をPRに寄せると、逆に回らなくなる場面がある。

• 複数の案から方針を選ぶとき
• 既存の構造を大きく変えるとき
• 意見が割れていて、文章だと長引きそうなとき

このへんは、PRを出す前に短く集まって方針だけ決めている。

決まったことはdocs/にMarkdownで置いて、それ自体もPRでレビューする。ドキュメントをリポジトリに置いてPRで見るのは標準的なやり方だが、「会議の結論をここに落とす」という線引きはこちらで決めたもの。

方向を決める話は会議、実装に落ちたあとの確認はPR、くらいの分け方に落ち着いた。

記録として使えるかは別に確認する — [今回の工夫]

正式な設計レビューが求められるプロジェクトだと、レビューをやった記録が必要になる。

PRベースにすると、こういうものが残る。ここまでは何もしなくてもGitHubが勝手に残してくれる。

• 誰がレビュアーに指名されたか
• 誰がいつ承認したか
• どんな指摘が出て、どう解決されたか
• マージ時点でどのチェックが通っていたか

ただ、これがそのまま正式な記録として通るかは、組織や適用する規格によって違うと思う。

必要な項目も保管期間も、こちらで勝手に決められるものではない。「GitHubに全部残っているから大丈夫」と思い込むのは、ちょっと怖い。

PRの履歴を記録そのものとして使うのか、一覧に転記するのかは、品質保証の担当と先に握っておいたほうがよさそう。
このすり合わせは機能の話ではないが、ここを飛ばすと、あとで全部やり直しになりかねない。

いきなり全部は切り替えない — [今回の工夫]

静的解析のときと同じで、最初から強く縛ると、仕組みのほうが邪魔者になる。
(それにいきなり変えると色々文句も産むものである。今までが慣れてる人からすると変えるコストの方が怖いのだ)

なので、こういう順番にした。ここは設定ではなく、完全に進め方の話になる。

1. しばらくは今までのレビューも残して、PRでも同じ内容を見る
2. CODEOWNERSは影響の大きい領域だけから始める
3. 必要な承認数は1件から
4. 困ったこと（PRの粒度、指摘の粒度、待ち時間）を集めて直す
5. 問題なさそうなところから、会議のほうを減らす

並行期間は単純に二重で手間が掛かる。それでも、「PRだと見落とす」が本当に起きるのかは、やってみないと分からない。

どこまでが標準機能だったか

最後に整理しておく。

やりたかったこと	標準機能で済むところ	こちらで決めたところ
見るべき人に見てもらう	CODEOWNERSによる自動指名	チーム単位で書く、狭く始める
見ないと通らないようにする	ブランチ保護の各設定	どれを入れるか、管理者を例外にしないこと
承認を意味のあるものにする	古い承認の無効化、コメント解決の必須化	既定オフのものを意識して入れる
説明不足を防ぐ	PRテンプレートの表示	何を書かせるか、粒度のお願い
設計方針を決める	（該当なし）	会議を残して、結果をdocsのPRにする
記録として使う	履歴が自動で残ること	記録として認められるかの確認

ざっくり言うと、機能はだいたい揃っていて、決めることのほうが多かった。設定を入れるだけなら半日で終わるが、その半日で終わらせると、たぶん承認だけが形として残る。

まとめ

設計レビューをPRに寄せるときに考えたことは、だいたい次の通り。

• 会議でやっていたことを分けて、移せるものだけ移す
• 誰が見るかはCODEOWNERS、見ないと通らないはブランチ保護で決める
• 効かせたい設定のいくつかは既定でオフなので、意識して入れる
• 方針を決める話は会議に残して、結果をドキュメントのPRにする
• 記録として使えるかは、自分たちだけで決めない
• 並行期間を置いて、様子を見ながら会議を減らす

レビューを「会議を開く作業」から「変更のたびに起きること」にできれば、設計を見る回数自体も増やせると思う。

参考

• About code owners - GitHub Docs
  • CODEOWNERSの書き方と、パターンの優先順位について
• About protected branches - GitHub Docs
  • 承認数、古い承認の無効化、Code Ownersの承認必須化などの設定