From 063aa24a013d2dde072bd509ec40e3cbe1404494 Mon Sep 17 00:00:00 2001 From: ma91n Date: Sat, 21 Dec 2024 22:42:45 +0900 Subject: [PATCH 1/7] Add git pr review flow --- .../forGitBranch/git_branch_standards.md | 53 ++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) 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したので、適時対応してマージして、といった運用が可能 | | 統制 | ✅️レビュアーが管理しやすい | ✅️メンバーの自主性に依存 | -| 要求スキル | ✅️低い。中央で統制を書けやすい | ⚠️開発メンバーの練度が求められる | +| 要求スキル | ✅️低い。中央で統制を行いやすい | ⚠️開発メンバーの練度が求められる | 上記にあるように、そのプルリクエストで実装した機能を、本番環境にデリバリーする責務をどちらに持たせるかという観点で、意思決定することが多い。 From 9f4edbf540637ddce7b5516b8347ceffebfe8821 Mon Sep 17 00:00:00 2001 From: ma91n Date: Sat, 21 Dec 2024 22:48:26 +0900 Subject: [PATCH 2/7] prettier fmt --- .../forGitBranch/git_branch_standards.md | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/documents/forGitBranch/git_branch_standards.md b/documents/forGitBranch/git_branch_standards.md index 3c208efc..192c2adc 100644 --- a/documents/forGitBranch/git_branch_standards.md +++ b/documents/forGitBranch/git_branch_standards.md @@ -272,13 +272,15 @@ featureブランチの開発が終わり、プルリクエストを作成する - マージ後のdevelopブランチでテスト失敗するリスクを減らすため - コンフリクトせずにマージ可能だったとしても、何かしらの依存関係や整合性が狂い、マージ後のテストが失敗する可能性がある -### tip プルリクエストのレビュー依頼までにどこまでテストしておくべきか +### プルリクエストのレビュー依頼までにどこまでテストしておくべきか 本規約で推奨する `Lite GitLab Flow` `GitLab Flow` ともに、開発環境へはdevelopマージをトリガーにCI/CDでデプロイを推奨している。 -そのため、プルリクエスト作成時点では開発環境（≒AWSなどクラウド環境の想定）へのデプロイ＋動作検証は不要とする考えである。 + +そのため、プルリクエスト作成時点では開発環境（≒AWSなどクラウド環境の想定）へのデプロイ＋動作検証は不要である。 + ローカルでの開発のみで品質担保が難しく手戻りが多い場合は、サンドボックス環境や開発環境にfeatureブランチからデプロイして動作検証する作業が必要になる。開発環境を共有する場合は、デプロイタイミングの制御がチーム内で必要になるため、運用ルールを検討する必要がある。 -### Terraformの場合、レビュー依頼までどこまで確認しておくべきか +### Terraformはレビュー依頼時点でどこまで確認しておくべきか Terraformはplanが成功しても、applyが失敗することは多々あり（サブネットが足りなかった、force_destory=trueを明示的な設定が必要だったなど）、レビューでの見極めは難しいことが多い。そのため、applyをどのタイミングで実施するかがチームの生産性の鍵となる。 @@ -293,16 +295,16 @@ Terraformはplanが成功しても、applyが失敗することは多々あり それぞれの特徴を下表にまとめる。 -| 観点 | ①マージ後に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 | ②Approve後にapply | ③CI以前に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の成功率が低い場合に有効 | 少数精鋭の場合に採用可能な、上級者向けの方式 | 本規約の推奨は以下。 From 933482956ec17e4708165e948a3751a593e74526 Mon Sep 17 00:00:00 2001 From: ma91n Date: Sun, 22 Dec 2024 04:42:35 +0900 Subject: [PATCH 3/7] npm run format --- .../forGitBranch/git_branch_standards.md | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/documents/forGitBranch/git_branch_standards.md b/documents/forGitBranch/git_branch_standards.md index 192c2adc..7df84293 100644 --- a/documents/forGitBranch/git_branch_standards.md +++ b/documents/forGitBranch/git_branch_standards.md @@ -295,16 +295,16 @@ Terraformはplanが成功しても、applyが失敗することは多々あり それぞれの特徴を下表にまとめる。 -| 観点 | ①マージ後にapply | ②Approve後にapply | ③CI以前に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 | ②Approve後にapply | ③CI以前に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の成功率が低い場合に有効 | 少数精鋭の場合に採用可能な、上級者向けの方式 | 本規約の推奨は以下。 @@ -346,7 +346,7 @@ Terraformはplanが成功しても、applyが失敗することは多々あり プリリクエストの承認（Approve）をもらった後、マージはレビュアー／レビュイーのどちらが行うべきか議論になる場合がある。 -| 観点 | レビュアー派 | レビュイー派 | +| 観点 | レビュアー派 | レビュイー派 | | ---------- | ------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | | 説明 | 開発者の責務が、developブランチにマージするまでという役割分担の場合に有効 | 各開発者がその機能のリリースについて責任を負うモデルの場合に有効 | | 生産性 | ⚠️レビュアーがブロッキングになりがち | ✅️高い。コメントはあるがApproveしたので、適時対応してマージして、といった運用が可能 | From 1831a88479152297cc239bb9d6954a5b91fbcf1a Mon Sep 17 00:00:00 2001 From: Junki Mano Date: Mon, 23 Dec 2024 15:19:04 +0900 Subject: [PATCH 4/7] Update documents/forGitBranch/git_branch_standards.md --- documents/forGitBranch/git_branch_standards.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documents/forGitBranch/git_branch_standards.md b/documents/forGitBranch/git_branch_standards.md index 7df84293..7558f50e 100644 --- a/documents/forGitBranch/git_branch_standards.md +++ b/documents/forGitBranch/git_branch_standards.md @@ -310,7 +310,7 @@ Terraformはplanが成功しても、applyが失敗することは多々あり - 新規参画者が多く統制を取りたい場合や、applyの成功率が高く維持できる場合は①を選択 - ある程度インフラメンバーが絞れ、かつapplyの失敗率が高くレビュー負荷が高くなってしまう懸念がある場合は②を選択 -- インフラメンバーが少数精鋭（通常、同時の作業はほぼ発生しない）の場合は必要に応じて、2をベースにしながら3を取り入れて生産性を上げる +- インフラメンバーが少数精鋭（通常、同時の作業はほぼ発生しない）の場合は必要に応じて、②をベースにしながら③を取り入れて生産性を上げる ## 2. featureブランチからdevelopブランチへ変更を取り込む From ec19130e434a8e30ede257f955fc37d7e3ef4dd9 Mon Sep 17 00:00:00 2001 From: Junki Mano Date: Mon, 23 Dec 2024 15:21:35 +0900 Subject: [PATCH 5/7] Update documents/forGitBranch/git_branch_standards.md --- documents/forGitBranch/git_branch_standards.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documents/forGitBranch/git_branch_standards.md b/documents/forGitBranch/git_branch_standards.md index 7558f50e..52cbc679 100644 --- a/documents/forGitBranch/git_branch_standards.md +++ b/documents/forGitBranch/git_branch_standards.md @@ -290,7 +290,7 @@ Terraformはplanが成功しても、applyが失敗することは多々あり - PR -> CI（planを含む） -> レビュー -> developマージ -> apply(CI) 2. Approve後にapply - PR -> CI（planを含む） -> レビュー -> apply -> developマージ -> apply(CI) -3. CI以前にapply +3. レビュー依頼前にapply - apply -> PR -> CI(plan含む) -> レビュー -> developマージ -> apply(CI) それぞれの特徴を下表にまとめる。 From 138e1409006b40e396ffa64f46d3ed534501ab04 Mon Sep 17 00:00:00 2001 From: Junki Mano Date: Mon, 23 Dec 2024 15:21:56 +0900 Subject: [PATCH 6/7] Update documents/forGitBranch/git_branch_standards.md --- documents/forGitBranch/git_branch_standards.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documents/forGitBranch/git_branch_standards.md b/documents/forGitBranch/git_branch_standards.md index 52cbc679..e5dd951b 100644 --- a/documents/forGitBranch/git_branch_standards.md +++ b/documents/forGitBranch/git_branch_standards.md @@ -295,7 +295,7 @@ Terraformはplanが成功しても、applyが失敗することは多々あり それぞれの特徴を下表にまとめる。 -| 観点 | ①マージ後にapply | ②Approve後にapply | ③CI以前にapply | +| 観点 | ①マージ後にapply | ②Approve後にapply | ③レビュー依頼前にapply | | -------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | ------------------------------------------------------------------------------- | | 説明 | developブランチにマージ後にapply。アプリコードと同じメンタルモデルを共有可能 | レビュアー承認後にapply。featureブランチからapplyするため、あるべき姿からは外れる | レビュー依頼前にapplyで成功したことを確認する方式 | | developブランチ品質 | ❌️一時的にapplyが失敗するコードが混入するリスク | ✅️apply可能なコードのみに保つことができる | ✅️apply可能なコードのみに保つことができる | From a695a2c0e0861127b5fba63d0ed7c0212aa485ac Mon Sep 17 00:00:00 2001 From: ma91n Date: Mon, 23 Dec 2024 15:24:25 +0900 Subject: [PATCH 7/7] np run format --- documents/forGitBranch/git_branch_standards.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documents/forGitBranch/git_branch_standards.md b/documents/forGitBranch/git_branch_standards.md index e5dd951b..ba4e0a1d 100644 --- a/documents/forGitBranch/git_branch_standards.md +++ b/documents/forGitBranch/git_branch_standards.md @@ -295,7 +295,7 @@ Terraformはplanが成功しても、applyが失敗することは多々あり それぞれの特徴を下表にまとめる。 -| 観点 | ①マージ後にapply | ②Approve後にapply | ③レビュー依頼前にapply | +| 観点 | ①マージ後にapply | ②Approve後にapply | ③レビュー依頼前にapply | | -------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | ------------------------------------------------------------------------------- | | 説明 | developブランチにマージ後にapply。アプリコードと同じメンタルモデルを共有可能 | レビュアー承認後にapply。featureブランチからapplyするため、あるべき姿からは外れる | レビュー依頼前にapplyで成功したことを確認する方式 | | developブランチ品質 | ❌️一時的にapplyが失敗するコードが混入するリスク | ✅️apply可能なコードのみに保つことができる | ✅️apply可能なコードのみに保つことができる |