Skip to content

Commit

Permalink
Add git pr review flow
Browse files Browse the repository at this point in the history
  • Loading branch information
@ma91n
ma91n committed Dec 21, 2024
1 parent fde5ef2 commit 063aa24
Showing 1 changed file with 51 additions and 2 deletions.
53 changes: 51 additions & 2 deletions documents/forGitBranch/git_branch_standards.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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を経由する運用を行うこと。
Expand Down Expand Up @@ -295,12 +344,12 @@ featureブランチでの作業中に、developブランチが更新された場

プリリクエストの承認（Approve）をもらった後、マージはレビュアー／レビュイーのどちらが行うべきか議論になる場合がある。

| 観点 | レビュアー派 | レビュイー派閥 |
| 観点 | レビュアー派 | レビュイー派 |
| ---------- | ------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ |
| 説明 | 開発者の責務が、developブランチにマージするまでという役割分担の場合に有効 | 各開発者がその機能のリリースについて責任を負うモデルの場合に有効 |
| 生産性 | ⚠️レビュアーがブロッキングになりがち | ✅️高い。コメントはあるがApproveしたので、適時対応してマージして、といった運用が可能 |
| 統制 | ✅️レビュアーが管理しやすい | ✅️メンバーの自主性に依存 |
| 要求スキル | ✅️低い。中央で統制を書けやすい | ⚠️開発メンバーの練度が求められる |
| 要求スキル | ✅️低い。中央で統制を行いやすい | ⚠️開発メンバーの練度が求められる |

上記にあるように、そのプルリクエストで実装した機能を、本番環境にデリバリーする責務をどちらに持たせるかという観点で、意思決定することが多い。

Expand Down

0 comments on commit 063aa24

Please sign in to comment.