本記事は【プルリクウィーク】 2日目の記事です。
💻 1日目 ▶▶ 本記事 ▶▶ 3日目 📚

はじめに

こんにちは、加藤です。

普段、私はインフラエンジニア（以下インフラ屋）としてシステム運用に携わっています。

最近はIaCの普及もあり、インフラチームでもプルリクエスト(以下PR)レビューを実施しているチームが多いのではないでしょうか？私が担当している案件でも、品質担保の観点からPRレビューを実施しております。私はレビュアーにもレビュイーにもなります。

本ブログ記事では、PRレビュー運用歴3年ほどのインフラ屋である私が、PRレビューを実施する中で難しく感じることや意識していることについて紹介したいと思います。

Git とインフラ屋と IaC

当然のことですが、PRレビューの運用にはGitの知識が必要です。

私見ですが、インフラ界隈ではGitはまだまだ浸透していない技術だと思っています。

私自身も今となっては当たり前のようにPRレビューを実施していますが、以前勤めていた会社ではインフラ分野のレビューといえば「作業手順書」や「パラメータシート」のレビューがほとんどでした。

もちろん、今でもそれらのドキュメントがシステム構築/運用保守のベースにあり、作成/更新/レビューすることは多くあります。

ただ、レビュー対象がそれらのドキュメントからPRに取って代わっていることは業務を通じて実感しております。（ドキュメント自体もGitで管理してしまう、ということもしばしば）

特に詳細な設定値については、ドキュメントではなくIaCで管理することがほとんどです。IaCで管理することで、ドキュメントの保守運用（往々にして陳腐化しがち）から解放されます。設定値を確認したい場合も、IaCは実環境と連動しているはず*1なので、ドキュメントを見るより確実です。

IaCを導入し承認フローを適切に定義することで、システム運用の品質は向上します。そのためにも、Gitはインフラ屋にとって必要なスキルになりつつあるのではないか、と考えています。操作ミスすると影響大きいですし、義務教育のカリキュラムに入っててもいいくらいだと、個人的には思ってます。

余談ですが、2024年2月にAWSから「AWS 規範ガイダンス」の一つとしてIaCツール比較ガイドが公開されました。

docs.aws.amazon.com

IaCの導入を検討されている場合は、上記を参考にしても良いかもしれません。

そもそもインフラ屋が管理するコードとは？

PRレビューの話に入る前に、インフラ屋が管理しているコードについて触れておきたいと思います。

ここで記載するコード群は私の業務を元にざっくりカテゴライズしたモノになります。チーム体制などで管理するコードの領域が変わるかと思いますので、一概にインフラ屋が管理するモノ、ということではない点はご了承ください。

IaC

まずは、IaCです。

上段でも触れましたが、IaCとはインフラ環境（特にクラウドやSaaS）をコードで管理するツールです。

TerraformやCDK、SAMなどが該当します。

Terraformは独自の言語（HCL）、CDKはプログラミング言語（TypeScript、Python、Java等）を用いてコードを実装します。SAMはCloudFormationテンプレートの拡張なのでJSON/YAMLで構成されます。

各ツールで必要スキルが異なるので、導入時は品質担保のためにもチームメンバーのスキルセットを考慮した上で選定すべきでしょう。

インフラ関連の設定ファイル

次にインフラ関連の設定ファイルです。

ミドルウェア（nginx等）の設定ファイルやDockerのDockerfile、KubernetesのManifestなどが該当します。

この手の設定ファイルはJSONやYAMLで表現されることが多いため構文で迷うことは少ないですが、各ツールの仕様やベストプラクティスを考慮する必要があります。（Dockerfileは独自の構文です）

また拡張機能やツール（Docker ComposeやHelmなど）を導入している場合は、それらの構成を理解する必要もあります。

IaCとは別のコード体系となるので、実環境への適用方法や管理リポジトリなどは別途検討する必要があります。

CI/CD周りの設定ファイル

インフラ関連の設定ファイルと似ていますが、業務上触る機会が多いので一つのカテゴリとしました。

GitHubActionsのWorkflowsファイル、AWS CodeBuildのbuildspec.yamlなどが該当します。

こちらもJSONやYAMLで書かれていることが多いです。

システムへの直接影響は少ないものの、問題が発生すれば開発作業が止まってしまうので重要なコードです。またアプリケーションに近い領域となるので、アプリケーションリポジトリに設置することが多いです。

各ツールでカスタマイズが必要な場合などは、シェルスクリプトで処理を実装することも多いです。それらの管理もインフラ屋がしています。

PRレビューで難しいと思うこと

上記のコード群に対してPRレビューを実施しているのですが、いろいろ難しく感じることがあります。

特にレビュアー視点で感じることについて、紹介していきます。

何を持ってOKとするか

まず、PRレビュー時に何を持ってOKとするか、という点です。

PRには下記のような項目を記載するかと思います。

課題と目的
検証方法と結果

レビュアーは上記とコード差分を確認して、目的を達成しているかを判断します。

検証は対応によって様々なのですが、ここの妥当性の判断が難しいことが多いです。

例えば、TerraformコードのPRをレビューする際はterraform planが想定通りのPlan結果であるか、という点を確認します。（Plan結果はPRに自動でコメントさせたりしてPR上で確認できる状態が望ましいです）しかし、想定通りのPlan結果であっても、いざterraform applyさせたところ、上手く動かない、ということは良くあります。

上手く動かない原因は、terraform applyが正常に完了しなかったり、Applyは出来ているが実環境の挙動が想定とは違ったり、などなどです。検証環境であれば良いですが、環境差分などが原因で本番環境しか発現しないこともあります。開発業務を円滑に進めるため、検証環境へのApplyはPR承認後ではなくPR承認前に開発者の自端末から実施し検証する、といった運用方針としている案件もあります。

また、検証方法は標準化されていないことが多いので、開発者の裁量（システムへの理解度やスキルセット、ノウハウなどから決定される）に依存することがあります。どれだけシステムを熟知していても人力での検証には抜け漏れが発生する可能性があるので、レビュイーが記載した検証方法や結果を鵜呑みにするのではなく、そもそもPRに記載されている情報が妥当か、という視点もレビュアーには必要かと考えています。

このような背景により、何を持ってOKとするか、という点について迷うことが多いです。

レビュアーはPRの目的を見失わず、広い視野でPRを評価する力が必要であると考えています。

そもそも検証が難しい

レビュー観点ではないですが、そもそも検証が難しいことも良くあります。

例えば、mainブランチへのマージをトリガーに実行するGitHubActionsワークフローの場合、実際にmainブランチへマージしてみないと検証は出来ません。検証用のリポジトリを作成して検証する、くらいしか検証方法がないのですが、できれば実運用に即した検証を実施したいところです。

また監視設定(Amazon CloudWatchやDatadogなど)を変更する際には、変更した監視機能が期待通りに動いているかを検証しておく必要があります。しかし、監視項目によっては検証作業はシステムの可用性を損ねる事態となりかねない(リソース監視設定の検証のために実際に高負荷をかける、など)ため、とりあえず監視の閾値を変更してアラーム状態となるか、くらいしか検証が出来ません。いざという時に必要な設定なので可能な限り検証はしておくべきかとは思います。

このような対応のPRをレビューする時は、どこまで検証するか、どのような観点を重要視するかなどを検討し、落とし所を探りつつ、納得感がある結論を打ち出していくことが必要になってくると考えています。

網羅性が判断つかない

上でも少し触れましたが、網羅性の判断も難しいです。

PRはコード差分のみが表示されるため、更新する必要があるコードが更新されていない場合、PRの情報だけでは判断がつきません。レビュイーの意図を汲み取りつつも結局リポジトリ内をgrepすることになります。

例えば、特定のシェルスクリプトを更新した場合に、単体での実行は検証済みだが、実は他のスクリプトから呼び出されており、その呼び出し先のスクリプトの実行はNGである、というケースです。

これらをPRレビューで見つけるためには、ある程度のコード体系の把握と技術者的な勘所が要求されます。網羅的な検証が自動で行えていれば良いですが、そういったインフラコード周りのCI技術はまだまだ発展段階なように思っています。（アプリケーションのCIはよく見ます）

PRレビューで意識していること

次に、PRレビューで意識している点についてです。

静的チェックの導入

インフラのCIは難しい、ということを書きましたが、静的チェックはLinter系ツールを導入することで対処するのが良いです。

Terraformであればtflint、JSONやYAMLはjsonlint、yamllintなどがあります。

これらはgit push(もしくはgit commit)時に自動でチェックされるように仕込んでおくことが望ましいです。静的チェックを導入することにより、ある程度のコードの標準化が可能となりPRレビューでの指摘事項の削減が見込まれます。（Linter系ツールを導入することで開発スピードが遅くなる懸念もあるため、導入時は要検討です）