【Gitブランチフロー規約】レビュー時のフローについて追記 (#193)

Author: ma91n

documents/forGitBranch/git_branch_standards.md

@@ -261,6 +261,57 @@ featureブランチでの作業中に、developブランチが更新された場
 
 :::
 
+### プルリクエスト作成前にアップストリームをプルする
+
+featureブランチの開発が終わり、プルリクエストを作成する際には、改めてアップストリーム（developブランチ）の変更をfeatureブランチに取り込み、差分が無いことを確認すべきである。
+
+理由は次の通り。
+
+- レビュアーの負荷軽減のため
+  - レビュアーがプルリクエストの差分以外の部分を参照した際に、それが古いバージョンであると、誤指摘、混乱してしまうなどの懸念がある
+- マージ後のdevelopブランチでテスト失敗するリスクを減らすため
+  - コンフリクトせずにマージ可能だったとしても、何かしらの依存関係や整合性が狂い、マージ後のテストが失敗する可能性がある
+
+### プルリクエストのレビュー依頼までにどこまでテストしておくべきか
+
+本規約で推奨する `Lite GitLab Flow` `GitLab Flow` ともに、開発環境へはdevelopマージをトリガーにCI/CDでデプロイを推奨している。
+
+そのため、プルリクエスト作成時点では開発環境（≒AWSなどクラウド環境の想定）へのデプロイ＋動作検証は不要である。
+
+ローカルでの開発のみで品質担保が難しく手戻りが多い場合は、サンドボックス環境や開発環境にfeatureブランチからデプロイして動作検証する作業が必要になる。開発環境を共有する場合は、デプロイタイミングの制御がチーム内で必要になるため、運用ルールを検討する必要がある。
+
+### Terraformはレビュー依頼時点でどこまで確認しておくべきか
+
+Terraformはplanが成功しても、applyが失敗することは多々あり（サブネットが足りなかった、force_destory=trueを明示的な設定が必要だったなど）、レビューでの見極めは難しいことが多い。そのため、applyをどのタイミングで実施するかがチームの生産性の鍵となる。
+
+大別すると以下の3方式が存在する
+
+1. マージ後にapply
+   - PR -> CI（planを含む） -> レビュー -> developマージ -> apply(CI)
+2. Approve後にapply
+   - PR -> CI（planを含む） -> レビュー -> apply -> developマージ -> apply(CI)
+3. レビュー依頼前にapply
+   - apply -> PR -> CI(plan含む) -> レビュー -> developマージ -> apply(CI)
+
+それぞれの特徴を下表にまとめる。
+
+| 観点                 | ①マージ後にapply                                                             | ②Approve後にapply                                                                 | ③レビュー依頼前にapply                                                          |
+| -------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | ------------------------------------------------------------------------------- |
+| 説明                 | developブランチにマージ後にapply。アプリコードと同じメンタルモデルを共有可能 | レビュアー承認後にapply。featureブランチからapplyするため、あるべき姿からは外れる | レビュー依頼前にapplyで成功したことを確認する方式                               |
+| developブランチ品質  | ❌️一時的にapplyが失敗するコードが混入するリスク                             | ✅️apply可能なコードのみに保つことができる                                        | ✅️apply可能なコードのみに保つことができる                                      |
+| レビュー負荷         | ❌️applyの成否は不明なので心理的負荷あり                                     | ❌️applyの成否は不明なので心理的負荷あり                                          | ✅️applyが成功している前提で対応可能。apply結果をコンソールからも確認可能       |
+| apply失敗時のコスト  | ❌️再度PRを作る必要があり手間                                                | ✅️同一PRを流用できる                                                             | ✅️apply成功後にPR作成が可能                                                    |
+| PRのトレーサビリティ | ❌️PRが割れると面倒                                                          | ✅️同一PRである                                                                   | ✅️同一PRである                                                                 |
+| 環境のバッティング   | ✅️ない                                                                      | ⚠️Approveからdevelopマージまでの間に、他メンバーの作業と重複するとややこしい      | ❌️作業調整が必要                                                               |
+| ガバナンス           | ✅️applyをCIのみに絞るなど自動化と相性が良い                                 | ⚠️レビュアー承認後のコードのみapply対象とできる                                   | ❌️ノーレビューのインフラ変更を適用するため、初学者が多いチームには適用が難しい |
+| 結論                 | applyの成功率が高く維持できる場合に有効                                      | applyの成功率が低い場合に有効                                                     | 少数精鋭の場合に採用可能な、上級者向けの方式                                    |
+
+本規約の推奨は以下。
+
+- 新規参画者が多く統制を取りたい場合や、applyの成功率が高く維持できる場合は①を選択
+- ある程度インフラメンバーが絞れ、かつapplyの失敗率が高くレビュー負荷が高くなってしまう懸念がある場合は②を選択
+- インフラメンバーが少数精鋭（通常、同時の作業はほぼ発生しない）の場合は必要に応じて、②をベースにしながら③を取り入れて生産性を上げる
+
 ## 2. featureブランチからdevelopブランチへ変更を取り込む
 
 プルリクエスト（以下、PR）を経由して、開発が完了したfeatureブランチをメインのdevelopブランチに取り込むためには、GitHub（GitLab）上でPRを経由する運用を行うこと。
@@ -295,12 +346,12 @@ featureブランチでの作業中に、developブランチが更新された場
 
 プリリクエストの承認（Approve）をもらった後、マージはレビュアー／レビュイーのどちらが行うべきか議論になる場合がある。
 
-| 観点       | レビュアー派                                                              | レビュイー派閥                                                                       |
+| 観点       | レビュアー派                                                              | レビュイー派                                                                         |
 | ---------- | ------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ |
 | 説明       | 開発者の責務が、developブランチにマージするまでという役割分担の場合に有効 | 各開発者がその機能のリリースについて責任を負うモデルの場合に有効                     |
 | 生産性     | ⚠️レビュアーがブロッキングになりがち                                      | ✅️高い。コメントはあるがApproveしたので、適時対応してマージして、といった運用が可能 |
 | 統制       | ✅️レビュアーが管理しやすい                                               | ✅️メンバーの自主性に依存                                                            |
-| 要求スキル | ✅️低い。中央で統制を書けやすい                                           | ⚠️開発メンバーの練度が求められる                                                     |
+| 要求スキル | ✅️低い。中央で統制を行いやすい                                           | ⚠️開発メンバーの練度が求められる                                                     |
 
 上記にあるように、そのプルリクエストで実装した機能を、本番環境にデリバリーする責務をどちらに持たせるかという観点で、意思決定することが多い。