こんにちは。ゲーム・エンターテインメント事業本部の鈴木です。
AndAppの開発をしています。
今回は私たちのチームで使っているreviewdogについて、CIの設定やlinterの組み合わせなど、どのようにしてコードレビューに活用しているか紹介します。
reviewdogとは
一言でいうとコードレビューを補助してくれるコマンドラインツールです。
各種linterの実行結果を渡してあげると、Pull Requestなどの差分に関して警告された行を教えてくれます。
私たちのチームではほとんどがGo言語による開発なため、golintなどの結果を表示するために使っています。
開発はGitHub Enterprise上で行なっており、CircleCI Enterpriseからreviewdogを実行すると以下のような警告がPull Requestのレビューコメントとして付きます。
似たようなツールであるDangerはPull Request全体に関わるチェックを得意としており、
- Pull RequestのTitleおよびDescription
- Pull Requestのファイル数や行数
- Pull Requestに含まれるファイル名
などを元にする時にはとても便利ですが、linterとの連携という点ではreviewdogの方が簡単に使えます。
なぜreviewdogを導入したのか
以下の3点からreviewdogの導入を決めました。
1. lintの結果をPull Requestのレビューコメントに表示できる
それまでは人が細かい部分までレビューをしていたため、そこに気を取られ過ぎてなかなか本質的なレビューに進めないということがありました。
そうなると双方が、細かい修正やその確認で消耗してしまい、レビュー自体が後回しになりがちという悪循環に陥っていました。
特にレビュアーへの時間や精神的な負担が大きく、その一部でも代行してくれるような仕組みが求められていました。
単純にlintの結果を表示するだけならDangerでも可能ですが、通常のコメントとして表示してしまうと何が警告されたのかわかりにくくなってしまいます。
かといって該当行に警告を表示しようとすると、様々なlinterの出力ごとに該当行を特定したり、APIを呼び出したりといった作り込みをしなければなりません。
reviewdogはそのあたりを吸収してくれるので人の代わりにレビューを行なってくれるツールとしてぴったりでした。
2. 改修のあった部分のみを警告できる
また、対象となるコードは既にサービスとしてリリースされていましたが、それまで一度もlinterをかけたことが無かったので大量の警告がある状態でした。
しかも開発は日々続いており、全てを直してからlinterを導入するのも現実的ではなく、改修のタイミングで段階的に対応していく必要がありました。
当然reviewdogはそのようなユースケースも想定されているのでPull Requestの差分に含まれない行の警告をフィルタリングしてくれます。
3. 任意のlinterを使用できる
最後は任意のlinterをかけたいという要求です。
AndAppではインフラにGoogle Cloud Platformを採用していますが、そのAPIの使い方を誤ってしまうことがありました。
こちらについては人によるチェックだと見逃されてしまうことがあるため、ツールを活用したいということをDeNA TechCon 2018でお話ししました。
しかし、既存のlinterでは私たちがチェックしたい内容を満たすものが無かったため、独自のlinterを作成してチェックするようにしました。
こういったケースでもreviewdogはlinterの出力をどのように扱うのかを簡単かつ柔軟に指定できるため、何の問題もなく独自のlinterを使うことができました。
reviewdogの使い方
Go製のツールなので、
go get -u github.com/haya14busa/reviewdog/cmd/reviewdog
もしくはGitHubのRleasesページからダウンロードしたバイナリをPATHの通った場所に配置することでインストールできます。
（CIで利用する場合、バージョンを固定できるこちらが推奨）
インストール後はREADME.mdやコマンドのhelpに書いてある通り、
$ <linter> | reviewdog [<flags>] (-reporter=github-pr-review | -diff=<diff command>)
の形式で実行できます。
例えばCircleCI上でgolintを実行する場合は以下のようになります。
export GITHUB_API=<GitHub EnterpriseのAPIエンドポイント> export REVIEWDOG_GITHUB_API_TOKEN=<APIアクセストークン> golint $(go list ./...) | reviewdog -f=golint -reporter='github-pr-review'
また、Pull Requestをpushする前などにローカルでgolintを実行する場合は以下のようになります。
golint $(go list ./...) | reviewdog -f=golint -diff='git diff master'
ただ、現在は以下にある複数のツールを使用しており、
それぞれに対して実行時にオプションを指定していたりと、個別に実行するのが面倒なのでまとめて実行するスクリプトを用意しています。
- go-reviewdog.sh
#!/bin/sh PKGROOT=$1 if [ "$1" = "" ]; then PKGROOT="." fi REVIEWDOG_ARG="-reporter='github-pr-review'" if [ "$CI_PULL_REQUEST" = "" ]; then REVIEWDOG_ARG="-diff='git diff master'" fi golint $(go list $PKGROOT/...) | eval reviewdog -f=golint $REVIEWDOG_ARG gsc -tests=false \ -exit-non-zero=false \ $(go list $PKGROOT/...) \ | eval reviewdog -f=golint -name="gsc" $REVIEWDOG_ARG megacheck -tests=false \ -staticcheck.exit-non-zero=false \ -simple.exit-non-zero=false \ -unused.exit-non-zero=false \ -unused.fields=false \ $(go list $PKGROOT/...) \ | eval reviewdog -f=golint -name="megacheck" $REVIEWDOG_ARG golangci-lint run \ --issues-exit-code 0 \ --out-format checkstyle \ --disable-all \ -E errcheck \ -E ineffassign \ -E interfacer \ -E unconvert \ -E misspell \ -E unparam \ -E nakedret \ -E prealloc \ $GOPATH/src/$PKGROOT/... \ | eval reviewdog -name=golangci-lint -f=checkstyle $REVIEWDOG_ARG golangci-lint run --tests=false \ --issues-exit-code 0 \ --out-format checkstyle \ --disable-all \ -E dupl \ -E goconst \ -E gocyclo --gocyclo.min-complexity 15 \ $GOPATH/src/$PKGROOT/... \ | eval reviewdog -name=golangci-lint -f=checkstyle $REVIEWDOG_ARG
こちらのスクリプトはローカルで実行する際はmasterとの差分をstdoutに出力します。
他にもBranchのpushなど、Pull Request以外の場合はそのような動作するようになっており、CircleCIでは次のように設定して使っています。
- .circleci/config.yml
version: 2 jobs: build: docker: - image: <reviewdogや関連ツール・スクリプトがインストール済みのイメージを指定> environment: GITHUB_API: <GitHub EnterpriseのAPIエンドポイント> # REVIEWDOG_GITHUB_API_TOKEN: CircleCI上のEnvironment Variablesに設定 steps: - checkout # 省略 - run: name: Execute reviewdog command: go-reviewdog.sh <対象のパッケージ名> # 省略
CIで利用するイメージは共通化されており、どのリポジトリでも同じlinterが同じルールで実行されます。
そのため、イメージの方でlinterやルールを変更することで、各リポジトリでは特に何もせずに新しい設定が適用されるようになっています。
linterについて
reviewdogを導入してから半年ほど経ちましたが、実はまだlinterやそのルールがしっかりと固まっていません。
基本的には、
- Goの標準的なスタイルに合わせる
golint
- シンプルで安定したコードを書く
megacheck
- よくある問題を防ぐ
gsc
としていますが、golangci-lintに関しては試行錯誤しながら使っています。
また、一部のlinterは実行時間の短縮や誤検出を減らすためにオプションを変えたりテストを除外しています。
定番のgo vetを入れていませんが、それは修正を必須としているのでreviewdogを通さずに実行しているためです。
試行錯誤した結果、現時点では下表のlinter設定にしています。
| Linter | Include tests | Description | |
|---|---|---|---|
| golint | - | 公式のlinter | |
| gsc | ⬜️ | スコープ外のContextを使用していないか、などのチェックをする | |
| megacheck | staticcheck | ⬜️ | 誤った動作をするコードがないかチェックする |
| gosimple | ⬜️ | より簡潔に書けるコードがないかチェックする | |
| unused | ⬜️ |
未使用の変数、型、フィールドがないかチェックする ※フィールドのチェックはgoonの _kindが引っかかるので無効にしている
|
|
| golangci-lint | errcheck | ✅ | 未チェックのエラーがないかチェックする |
| ineffassign | ✅ | 値が未使用なまま上書きされている変数がないかチェックする | |
| interfacer | ✅ | より小さいinterfaceに置き換えられる引数がないかチェックする | |
| unconvert | ✅ | 不必要なキャストがないかチェックする | |
| misspell | ✅ | 英単語のスペルに誤りがないかチェックする | |
| unparam | ✅ | 未使用の引数がないかチェックする | |
| nakedret | ✅ | Named Result Parametersによるreturnをしていないかチェックする | |
| prealloc | ✅ | 事前に容量を確保できるスライスがないかチェックする | |
| dupl | ⬜️ | 重複しているコードがないかチェックする | |
| goconst | ⬜️ | 定数に置き換えられるコードがないかチェックする | |
| gocyclo | ⬜️ |
コードの循環的複雑度をチェックする ※ min-complexityを15に設定（デフォルトは30）
|
|
この中でよく警告されるのはコメントが無いコードに対してgolintが出力する
exported XXX should have comment or be unexported
ですが、コメントの英語縛りはしていないため、警告されたら日本語でコメントを足せばいいのであまり困っていません。
それにコメントを書くタイミングで書きづらいことに気づいた場合、処理を詰め込みすぎている可能性があるので実装を考え直すきっかけにもなっています。
まとめ
reviewdog導入の成果として「コードの品質を改善しよう」という意識がチーム内で高まりました。
おかげで最低限のチェックが済んだ状態でレビューを始めることができるようになりました。
ただし、一部のlinterは厳しめのチェックをすることもあるため、実装者にとってはやや負担が増えてしまっているようにも感じます。
それでも後々になって致命的な問題が見つかるよりはよっぽど良いと思っています。
また、reviewdogの特徴として改修のあった部分のみが警告されるため、自分の触ったコードやその周辺を綺麗にしようという「ボーイスカウト・ルール」が働きやすくなっています。
これが1度に全ての警告が出てきてしまうと気軽に直すのが難しく、つい後回しにされがちなので狙い通りの効果だと言えます。
今の段階ではまだ誤検出をしてしまったり、警告の意味がわからず戸惑ってしまうようなことがあるような状況です。
しかし、今後もlinterやルールは継続的に見直していくつもりですし、慣れてくると意識しないでも警告されないコードが書けるようになるはずです。
このような積み重ねをしてくことでレビューの効率化、ひいては全体的なGo力の底上げに繋げていきたい考えています。
