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