コードレビューガイドライン

このガイドには、コードレビューを実施したり、コードをレビューしてもらったりするためのアドバイスやベストプラクティスが記載されています。

GitLab CEとEEへのマージリクエストは、GitLabチームメンバーによって書かれたものであれ、より広いコミュニティメンバーによって書かれたものであれ、コードが効果的で、理解可能で、メンテナーで、セキュリティが確保されていることを確認するために、コードレビュープロセスを通らなければなりません。

マージリクエストをレビュー、承認、マージするには

始める前に

レビューするコードができたら、すぐにレビュアーに レビューしてもらいましょう。このレビュアーは、あなたのグループやチームの人でも、ドメインの専門家でもかまいません。レビュアーは次のことができます：

  • 選択したソリューションと実装について、セカンドオピニオンを与えること。
  • バグ、ロジックの問題、エッジケースの発見を支援します。

マージリクエストが小さくてレビューが簡単なものであれば、レビュアーのステップを省略して直接メンテナーに質問することができます。

何をもって「小さくて簡単」とするかはグレーゾーンです。小さくて簡単な変更の例をいくつか挙げます：

  • 誤字脱字の修正や、ちょっとしたコピーの変更（例）。
  • 動作やデータを変更しない小さなリファクタリング。
  • 1ヶ月以上デフォルトで有効になっている機能フラグへの参照を削除しました。
  • 未使用のメソッドまたはクラスの削除。
  • よく理解されているロジックの変更で、5行未満のコードの変更を必要とするもの。

そうでない場合、マージリクエストはまず、MRが触れる各カテゴリ（例えばバックエンドやデータベース）のレビュアーによってレビューされるべきです。これは、作業負荷を分散することにも役立ちます。

セキュリティスキャンやコメントの支援については、アプリケーションセキュリティチーム (@gitlab-com/gl-security/appsec) を含めてください。

レビュアーはサイドバーのレビュアー機能を使います。レビュアーは追加承認することで、承認を追加することができます。

マージリクエストが触れる領域によっては、1人以上のメンテナーの 承認が必要です。承認者ボタンはマージリクエストウィジェットにあります。

マージリクエストをマージするにはメンテナーも必要です。複数の承認者が必要な場合、最後にレビューして承認したメンテナーがマージします。

いくつかのドメインエリア(のようなVerify)では、CODEOWNERSルールに基づいて、ドメインの専門家の承認者が必要です Verify。VerifyCODEOWNERSセクションは独立した承認ルールなので Verify、より一般的な承認ルール(たとえばbackend)のサブセットとなるようなVerify特定のルール(たとえば Verify)をVerify持つ Verifyことができます。より効率的なプロセスのために、作成者は一般的な承認の前にドメイン固有の承認を探すべきです。ドメイン固有の承認者はメンテナーでもあり得ます。もしそうなら、ドメイン固有の変更と広範な変更を同時にレビューし、両方のロールに対して一度承認するべきです。

作成者の責任については以下をご覧ください。

ドメインの専門家

ドメイン・エキスパートとは、特定の技術、製品機能、コードベースの領域について豊富な経験を持つチーム・メンバーのことです。チームメンバーは、ドメインエキスパートを自認し、チームプロフィールに追加することが推奨されます。

ドメイン・エキスパートを自認する場合は、.yml ファイルを変更する MR を、すでに確立されているドメイン・エキスパートまたは対応するエンジニアリング・マネージャーに割り当てることを推奨します。

自動的にドメイン・エキスパートとみなされることに関しては、以下のように仮定します：

  • 特定のステージ/グループ（たとえば、作成：ソースコード）で作業しているチームメンバーは、作業しているアプリのその分野のドメインエキスパートとみなされます。
  • 特定の機能（たとえば、検索）に取り組んでいるチームメンバーは、その機能のドメインエキスパートとみなされます。

コードレビューの場合、レビューの割り当てはデフォルトでドメインの専門家であるチームメンバーに割り当てられます。UXレビューは、レビュールーレットから推奨されるレビュアーにデフォルトで割り当てられます。デザイナーのキャパシティの制限により、プロダクトデザイナーがサポートしない領域は、コミュニティ貢献でない限り、UXレビューが不要になりました。適切なドメイン エキスパートがいない場合、任意のチーム メンバーを MR レビューに選択するか、レビュアー ルーレットの推奨に従うことができます (UX レビューについては上記を参照)。

ドメインエキスパートを見つけるには

  • マージリクエストの承認者 ウィジェットで、資格のある承認者を表示 を選択します。このウィジェットには、コードベースの領域ごとに推奨される承認者と必要な承認者が表示されます。これらのルールはコード オーナーで定義されます。
  • マージ リクエストに関連するステージまたはグループで作業するチーム メンバーのリストを表示します。
  • エンジニアリングプロジェクトページか GitLab チームページで、チームメンバーのドメイン専門知識を見ることができます。ドメインは自分で決めるものなので、あなたの判断でマージリクエストの変更をドメインにマッピングしてください。
  • マージリクエストのファイルに貢献したチームメンバーを探します。git log <file> を実行してログを表示します。
  • ファイルをレビューしたチームメンバーを探します。関連するマージリクエストは次の方法で見つけることができます：
    1. git log <file> を使ってコミット SHA を取得します。
    2. https://gitlab.com/gitlab-org/gitlab/-/commit/<SHA> への移動 .
    3. コミットに表示されている関連マージリクエストを選択します。

レビュアー・ルーレット

note
レビュアー・ルーレットはGitLab.comで使用する内部ツールであり、顧客のインストールでは使用できません。

Danger ボットは、あなたのマージリクエストが触れそうなコードベースの各領域について、レビュアーとメンテナーをランダムに選びます。開発者のレビュアーについては推奨されますが、他の人のほうが適していると思う場合はそれを上書きしてください。

プロダクトデザイナーを含むチームからのMRに対してのみ、UXレビューを行います。これらのチームからのユーザー向けの変更には、機能フラグの後ろであってもUXレビューが必要です。推奨されるUXレビュアーがデフォルトで提案されます。

エンジニアリングプロジェクトページのリストからレビュアーとメンテナーを選択します：

  • SlackやGitLabのステータスを持つ人を選びません：
    • 文字列OOO,PTO,Parental Leave, またはFriends and Family を含む人を選びません。
    • GitLab ユーザーBusyインジケータはTrue に設定されています。
    • 絵文字はこれらのカテゴリのいずれかです：
      • 休暇中- 🌴:palm_tree:, 🏖️:beach:, ⛱:beach_umbrella:, 🏖:beach_with_umbrella:, 🌞:sun_with_face:, 🎡。:ferris_wheel:
      • 病欠- 🌡️:thermometer:, 🤒。:face_with_thermometer:
      • 定員に達している - 🔴 療養中:red_circle:
      • 集中モード- 💡:bulb: (チームの仕事に集中)
  • 選択した「レビュー制限」以上のレビュー数をすでに割り当てられている人を選ぶことはありません。レビュー制限とは、一度に処理できるレビュー数の上限です。SlackやGitLabのステータスとして、以下のいずれかを使ってレビューの上限を設定します：
    • 0️⃣ -:zero: (:red_circle: と同様)
    • 1️⃣ -:one:
    • 2️⃣ -:two:
    • 3️⃣ -:three:
    • 4️⃣ -:four:
    • 5️⃣ -:five:

    セキュリティグループ配下のプロジェクトのデフォルトブランチを対象としていないマージリクエストのレビューリクエストはカウントされません。このような MR は通常バックポートであり、メンテナーやレビュアーは通常レビューにあまり時間を必要としません。

  • Slack やGitLab のステータスの絵文字が 🔵:large_blue_circle: のチームメンバーは、選ばれる可能性が高くなります。これはレビュアーにも研修メンテナーにも当てはまります。
    • 🔵:large_blue_circle: のレビュアーは、他のレビュアーの2倍の確率で選ばれます。
    • :large_blue_circle: 🔵のメンテナー研修生は、他のレビュアーより3倍選ばれやすい。
  • GitLabステータスの絵文字が🔶:large_orange_diamond: または🔸:small_orange_diamond: の人は、選ばれる確率が半分になります。
  • 同じブランチ名のレビュアーやメンテナーは、常に同じ人を選びます (ただし、ポイント 1 のようにオフィス外 (OOO) のステータスが変更された場合は除きます)。先頭のce- とee- 、末尾の-ce と-eeを削除し、バックポートブランチでも安定するようにします。
  • Slack やGitLab のステータスの絵文字が Ⓜ:m:の人は、メンテナーであるプロジェクトのレビュアーとしてのみ提案されます。

ルーレットのダッシュボードには次のようなものがあります：

  • 過去7日間と30日間の割り当てイベント。
  • 現在割り当てられている1人あたりのマージリクエスト。
  • さまざまな基準でソートします。
  • 手動レビュアー・ルーレット。
  • 現地時間情報。

詳しくはroulette README をレビューしてください。

承認者ガイドライン

以下のメンテナーの責任のセクションで説明されているように、ドメインの専門知識を持つメンテナーによってマージリクエストを承認してもらい、マージすることをお勧めします。最初のレビュアーによる承認 (オプション) についてはここでは扱いません。しかし、あなたのマージリクエストは、概要のセクションで説明されているように、メンテナーに渡す前にレビュアーによってレビューされるべきです。

もしあなたのマージリクエストにによる承認者が必要です。
~backend 変更1 バックエンドのメンテナー。
~database マイグレーションや高価なクエリの変更2 データベースのメンテナー。詳細はデータベースレビューガイドラインを参照してください。
~workhorse 変更 Workhorseメンテナー。
~frontend 変更1 フロントエンドメンテナー。
~UX ユーザー向けの変更3 製品デザイナー。詳細については、デザインおよびユーザー インターフェイスのガイドラインを参照してください。
新しい JavaScript ライブラリの追加1 -
- 新しいライブラリが使用するライセンスが GitLab での使用を承認されていない場合、法務部門の メンバー。

ライセンスの互換性についての詳細はGitLab Licensing and Compatibility ドキュメントをご覧ください。
新しい依存関係やファイルシステムの変更-ディストリビューションチームメンバー。詳細はディストリビューションチームとの連携方法をご覧ください。
- RubyGems については、AppSec レビューをリクエストしてください。
~documentation または~UI text を変更してください。適切なDevOpsステージグループでのアサインに基づくテクニカルライター。
開発ガイドラインの変更 レビュアー・プロセスに従い、承認者の承認を得てください。
エンドツーエンドおよび非エンドツーエンドの変更4 テストのソフトウェアエンジニア。
エンドツーエンドの変更のみ4 または、MR作成者がテスト中のソフトウェアエンジニアの場合 品質メンテナー。
新規または更新されたアプリケーションの制限 プロダクトマネージャー
アナリティクス・インスツルメンテーション（テレメトリーまたはアナリティクス）の変更 Analytics Instrumentationエンジニア。
フィーチャースペックの追加・変更 品質メンテナーまたは品質レビュアー。
GitLabの新しいサービス（Puma、Sidekiq、Gitalyがその例です。） プロダクトマネージャー。詳細はGitLabへのサービスコンポーネントの追加プロセスをご覧ください。
認証や作成者に関する変更 管理:認証・認可チームメンバー。詳細については、グループページのコードレビューセクションを確認してください。チームからのレビューが必要であることが知られているファイルのパターンはCODEOWNERS ファイルのAuthentication and Authorization セクションにリストされ、チームはこれらのファイルを変更するすべてのマージリクエストの承認者セクションにリストされます。
  1. JavaScript 仕様以外の仕様は~backend コードとみなされます。Haml マークアップは~frontend コードとみなされます。ただし、Haml テンプレート内の Ruby コードは~backend コードとみなされます。疑わしい場合は、フロントエンドとバックエンドの両方のレビューを依頼してください。
  2. マージリクエストが高価なクエリを導入する可能性がある場合は、データベースメンテナーの指導を仰ぐことをお勧めします。SQL クエリを含む問題のコード行をコメントするのが最も効率的です。
  3. ユーザー向けの変更には、視覚的な変更（どんなに些細なものであっても）と、スクリーンリーダーがコンテンツをどのようにアナウンスするかに影響を与えるレンダリングDOMの変更の両方が含まれます。専任のプロダクトデザイナーがいないグループは、変更がコミュニティ貢献者でない限り、機能変更を承認するためにプロダクトデザイナーを必要としません。
  4. エンドツーエンドの変更には、qa ディレクトリ内のすべてのファイルが含まれます。

受け入れチェックリスト

このチェックリストは、マージリクエスト（MR）の作成者、レビュアー、メンテナーに対して、変更が品質、パフォーマンス、信頼性、セキュリティ、観測可能性、保守性などに大きな影響を与えるリスクについて分析されていることを確認するよう促すものです。

チェックリストの使用はソフトウェアエンジニアリングの品質を向上させます。このチェックリストは、GitLabコードベースへの貢献者のスキルをサポートし、強化するためのわかりやすいツールです。

品質

品質に関する詳しいガイドラインについては、テストエンジニアリングプロセスをご覧ください。

  1. あなたは、コードレビューのガイドラインに従って、この MR をセルフレビューしました。
  2. この変更が影響するコードについて、あなたは自動テスト（テストガイド）がユーザーにとって非常に重要な機能を検証していると考えています（すべてのテストレベルの考慮を含む）。
  3. 既存の自動テストが上記の機能をカバーしていない場合、必要な追加テストを追加するか、自動テストのギャップを記述するイシューを追加し、この MR にリンクしました。
  4. あなたは、この変更が GitLab.com をホストしている顧客や自己管理している顧客に与える影響を技術的な側面から考慮しました。
  5. この変更がシステムのフロントエンド、バックエンド、データベース部分に与える影響を考慮し、~ux、~frontend 、~backend、~database のラベルを適切に適用しました。
  6. サポートされているすべてのブラウザでこの MR をテストしたか、またはこのテストが必要ないと判断しました。
  7. この変更がアップデート間で後方互換性があることを確認したか、または適用されないと判断しました。
  8. EEコンテンツとFOSSを適切に分離したか、またはこのMRはFOSSのみです。
  9. 既存のデータが驚くほど多様であることを考慮しましたか？例えば、新しいモデルのバリデーションは既存のレコードを壊す可能性があります。既存データがバリデーションをパスすることを確認していない場合は、既存データのバリデーションを必須ではなくオプションにすることを検討してください。
  10. テストが警告付きで合格し、不合格のジョブにFlaky test '<path/to/test>' was found in the list of files changed by this MR.というテキストが含まれている場合、このテストを修正したか、あるいは、この欠陥のあるテストが無視できる理由を説明する証拠を提供したことになります。
パフォーマンス、信頼性、可用性
  1. この MR がパフォーマンスに悪影響を与えないことを確信しているか、パフォーマンスへの影響を評価するためにレビュアーに依頼しています。(マージリクエストパフォーマンスガイドライン)
  2. MR の説明にデータベースレビュアー向けの情報を追加したか、または不要と判断しました。
  3. この変更による可用性と信頼性のリスクを考慮しましたか？
  4. 将来予測される成長に基づくスケーラビリティ・リスクを考慮しましたか。
  5. 平均的な顧客よりもはるかに多くのデータを持つ可能性のある大規模な顧客に対する、この変更によるパフォーマンス、信頼性、および可用性の影響を考慮しました。
  6. あなたは、最小限のシステムで GitLab を実行する可能性のある顧客に対するこの変更のパフォーマンス、信頼性、可用性の影響を考慮しました。
観測可能なインストルメンテーション
  1. デバッグを容易にし、観測可能性によって積極的にパフォーマンスを改善するのに十分なインスツルメンテーションが含まれています。機能フラグ、ロギング、インスツルメンテーションの追加例を参照してください。
ドキュメント
  1. 変更履歴トレイラーを含むか、あるいは必要ないと判断しました。
  2. ドキュメントを追加/更新したか、またはこの MR にドキュメントの変更は不要であると判断しました。
セキュリティ
  1. この MR に、認証情報またはトークン、作成者、および認証方法、またはセキュリティ・レビュー・ ガイドラインに記載されているその他の項目の処理または保存に対する変更が含まれている場合、~security のラベルを追加し、@-@gitlab-com/gl-security/appsec を記載したことを確認しました。
  2. あなたは、いつ、どのようにセキュリティレビューを要求するかについて、内部アプリケーションのセキュリ ティレビューに関する文書を確認し、この変更についてセキュリティレビューが必要である場合は、セキュリ ティレビューを要求しました。
  3. MR をブロックしているセキュリティ・スキャン結果がある場合（スキャン結果ポリシーによる）：
    • 真正の発見については、マージリクエストをマージする前に修正する必要があります。これにより、スキャン結果ポリシーによって要求される AppSec 承認者が削除されます。
    • 誤検出、リスク受容のために議論されるべきもの、または疑わしいものについては、@gitlab-com/gl-security/appsecに ping してください。
デプロイ
  1. この変更はリスクが高い可能性があるため、機能フラグを使用することを検討しました。
  2. 機能フラグを使用する場合、本番環境でテストする前にステージングでその変更をテストする予定であり、すべての顧客にロールアウトする前に本番顧客のサブセットにロールアウトすることを検討しています。
  3. インフラストラクチャー部門に、デフォルト設定や新しい設定の変更を、完了の定義ごとに通知した場合、またはその必要がないと判断した場合。
コンプライアンス
  1. 正しいMR タイプラベルが貼付されていることを確認しました。

マージリクエスト作成者の責任

最善の解決策を見つけ、それを実装する責任はマージリクエスト作成者にあります。作成者または直接責任を負う個人 (DRI) は、コードレビューのライフサイクルを通して担当者としてマージリクエストに割り当てられたままです。担当者として設定できない場合は、レビュアーに依頼してください。

承認者とマージするためにレビュアーにレビューをリクエストする前に、レビュアーは次のことを確信する必要があります：

  • 解決しようとした問題を実際に解決していること。
  • 最も適切な方法で。
  • すべての要件を満たします。
  • バグ、論理的問題、エッジケースの未発見、既知の脆弱性はありません。

これを行う最善の方法は、レビュアーとの不要なやり取りを避けるために、コードレビューのガイドラインに従って、自分自身のマージリクエストのセルフレビューを行うことです。このセルフレビューでは、決定やトレードオフが行われた行や、文脈に沿った説明がレビュアーがコードを理解しやすくなるような行について、MRにコメントを含めるようにしてください。

作成者は、自分の解決策に必要なレベルの信頼を得るために、調査や実装のプロセスに他の人を適宜参加させることが期待されています。

異なるソリューションについて議論したり、実装をレビューしてもらうためにドメインの専門家に、混乱を解消したり、最終結果が自分の考えていたものと一致しているか確認するためにプロダクトマネージャやUXデザイナーに、データモデルや特定のクエリについて意見をもらうためにデータベースの専門家に、あるいはソリューションの詳細なレビューを得るために他の開発者に連絡を取ることが推奨されます。

マージリクエストが複数のドメイン（たとえば、Dynamic AnalysisとGraphQL）にまたがる場合は、各ドメインの専門家にレビューを依頼してください。

作成者がマージリクエストにドメインエキスパートの意見が必要かどうかわからない場合、それは必要であることを示しています。それがなければ、彼らのソリューションに必要なレベルの自信があるとは思えません。

レビューの前に、作成者はマージリクエストの差分に関するコメントを提出するよう求められます。コメントを必要とする内容の例としては、次のようなものがあります：

  • linting ルール (RuboCop、JS など) の追加。
  • ライブラリの追加 (Ruby gem, JS lib など)
  • 明らかでない場合は、親クラスやメソッドへのリンク。
  • 変更を補完するために実行されたベンチマーク。
  • 潜在的に安全でないコード。

レビュアーがソリューションを検証するために必要なプロジェクト、スニペット、その他の資産がある場合は、レビューを依頼する前に、それらの資産にアクセスできるようにしてください。

レビュワーを割り当てる際には、以下のことが役立ちます：

  • レビュアーにどのようなタイプのレビューを求めるか、MRにコメントを追加してください。
    • 例えば、MR がデータベースクエリを変更し、バックエンドコードを更新する場合、MR 作成者はまず~backend と~database のレビューが必要です。作成者はレビュアーを割り当てると同時に、MR にコメントを追加し、各レビュアーがどのドメインのレビューを行うべきかを知らせます。
    • 多くのGitLabチームメンバーは複数の領域の専門家であるため、このようなコメントがないと、どのような種類のレビューを求められているのかが曖昧になることがあります。
    • MRレビューの種類を明確にすることは、MR作成者にとっては自分が求めているレビューの種類を受け取ることができるので効率的であり、MRレビュアーにとってはどの種類のレビューを提供すべきかがすぐにわかるので効率的です。
    • 例 1
    • 例 2

避けてください：

  • レビュアーからの要求がない限り、TODOコメント（上記参照）をソースコードに直接追加してください。アクション可能なタスクのためにTODOコメントを追加する場合は、関連するイシューへのリンクを含めてください。
  • コードが何をしているかを説明するだけのコメントを追加します。TODOでないコメントを追加する場合は、次のようにします。 を説明するのではなく、なぜ.
  • テストが失敗したマージリクエストのメンテナーのレビューを要求すること。テストが失敗し、レビュアーにレビューを依頼する必要がある場合は、 必ずコメントを残して説明するようにしてください。
  • メールやSlackでメンテナーについて過度に言及すること (メンテナーがSlackで連絡可能な場合)。マージリクエストにレビュアーを追加できない場合、@ コメントでメンテナーについて言及することは許容されます。

これによりレビュアーが時間を節約でき、作成者がミスを早く発見できるようになります。

レビュアーの責任

レビュアーは、選択されたソリューションの詳細をレビューする責任があります。

マージリクエストを徹底的にレビューしてください。

マージリクエストがすべての貢献受諾基準を満たしていることを確認してください。

マージリクエストの中には、ドメインエキスパートに詳細な説明を求めるものもあります。レビュアーは、その分野の専門家でない場合、次のいずれかを行うことができます：

  • マージリクエストをレビューし、ドメインエキスパートに別のレビューを依頼します。この専門家は別のレビュアーでもメンテナーでもかまいません。
  • より適切と思われる別のレビュアーにレビューを回します。
  • ドメインの専門家がいない場合は、ベストエフォートでレビューしてください。

もしそうであれば、マージリクエストをより小さなマージリクエストに分割するように作成者を導いてください：

  • 大きすぎます。
  • 複数のイシューを修正。
  • つ以上の機能を実装しています。
  • 複雑性が高く、さらなるリスクを伴うもの。

作成者は、現在のメンテナーとレビュアーに分割MRのレビューを依頼するか、新しいメンテナーとレビュアーのグループを依頼するかを選択できます。

すべての要件を満たしていると確信が持てたら、そうすべきです：

  • 承認者を選択します。
  • @ 作成者にTo-Do通知を送信し、マージリクエストがレビューされ承認されたことを通知します。
  • メンテナーにレビューを依頼します。デフォルトではドメインの専門知識を持つメンテナーにリクエストしますが、メンテナーがいない場合や、マージリクエストにドメインの専門家によるレビューが必要ないと思われる場合は、レビュアルーレットの提案に従ってください。
  • あなた自身をレビュアーから外してください。

メンテナーの責任

メンテナーは、GitLabコードベースの全体的な健全性、品質、一貫性に責任を負います。

その結果、彼らのレビューは主に全体的なアーキテクチャ、コード構成、懸念事項の分離、テスト、DRYネス、一貫性、可読性などに焦点を当てます。

メンテナーのジョブはGitLabのコードベース全体に関する知識だけに依存し、特定のドメインに関する知識には依存しないため、どのチーム、どのプロダクト領域のマージリクエストでもレビュー、承認、マージすることができます。

メンテナーは、マージリクエストの受け入れ基準が合理的に満たされていることを保証するDRIです。一般的に、品質はすべての人の責任ですが、MR のメンテナーは MR が一般的な品質基準を満たしていることを保証する責任があります。

これには、フォローアップのイシューで技術的負債を作らないようにすることも含まれます。

もしメンテナーがMRの内容が十分なものであると感じたり、ドメインの専門家が必要であると感じたりした場合、メンテナーには他のレビュアーやメンテナーにレビューを依頼する裁量権があります。以下は、メンテナーがレビュー中に積極的にこのようなことをしている例です：

メンテナーはマージする前に、選択されたソリューションの詳細もレビューするよう最善を尽くしますが、必ずしもドメインの専門家ではないので、無理な時間を投資しなければレビューできないかもしれません。そのような場合は、本来の責務に集中するために、作成者や以前のレビュアーたちの判断に従います。

たまたまメンテナーでもある開発者がレビュアーとしてマージリクエストに関わった場合、最終的に承認してマージするメンテナーには選ばないことが推奨されます。

メンテナーはマージする前に、マージリクエストが必要な承認者によって承認されているかどうか確認してください。まだ他の人の承認を待っている場合は、あなた自身をレビュアーから外し、@ 、作成者に言及し、その理由をコメントで説明してください。コードをマージする場合はレビュアーでいてください。

マージリクエストの中には安定版ブランチを対象とするものもあります。このようなリクエストの扱い方の概要については、パッチリリースのランブックを参照してください。

マージ後、メンテナーはマージリクエストに記載されたレビュアーとして残るべきです。

レビュアー機能のドッグフード化

2021年3月18日、レビュアー機能の効率的かつ一貫したドッグフードを目的とした最新のプロセスが導入されました。

以下は、このセクションにも反映されている変更点の概要です。

  • マージリクエスト作成者とDRIは担当者のままです。
  • 作成者はユーザーをレビュアーとして割り当てることで、レビューをリクエストします。
  • レビュアーはレビューと承認が終わると、割り当てを解除します。
  • 最後の承認者（MRをマージする者）はレビュアーとして割り当てられたままです。

ベストプラクティス

皆さん

  • 親切にしましょう。
  • 多くのプログラミングの決定は意見であることを受け入れましょう。トレードオフについて議論し、どちらを優先するかを考え、素早く解決しましょう。
  • 質問をしてください。(「この:user_id という名前はどう思いますか?”)
  • 説明を求めます。(「理解できなかったのですが、明確にしてもらえますか？)
  • コードの選択的所有権を避けること。(「私の」、「私のものではない」、「あなたの」)
  • 個人的な特徴を指していると見なされるような用語の使用は避けてください。(“dumb”、”thupid”).誰もが知的で善意を持っていると仮定しましょう。
  • 明確に。ネット上では、必ずしもあなたの意図が理解されるとは限らないことを忘れないでください。
  • 謙虚に。(よく分からないので調べてみましょう。）
  • 大げさな表現を使わないこと。(「いつも」、「決して」、「果てしなく」、「何もない」)
  • 皮肉の使用には注意しましょう。私たちの行動はすべて公開されています。あなたや長年の同僚にとっては気さくな皮肉に見えても、プロジェクトに参加したばかりの人にとっては意地悪で歓迎されていないように映るかもしれません。
  • 理解できなかった」「代替案：」というコメントが多い場合は、1対1のチャットやビデオ通話を検討してください。1対1の議論をまとめたフォローアップコメントを投稿しましょう。
  • 特定の人に質問する場合は、必ずその人について言及することからコメントを始めましょう。こうすることで、その人の通知レベルが「言及済み」に設定されている場合、その人は確実にそのコメントを見ることができ、他の人は返信する必要がないことを理解します。

マージリクエストのレビュー

コードレビューは何度も繰り返されるプロセスであり、レビュアーは最初見たときには気づかなかったことを後で発見するかもしれないことを覚えておいてください。

  • あなたのコードの最初のレビュアーは_あなた_です。ピカピカのブランチを最初にプッシュする前に、差分全体に目を通しておきましょう。それは理にかなっていますか?変更の全体的な目的とは関係のないものが含まれていませんか?デバッグ用のコードを削除し忘れていませんか?
  • マージリクエストガイドラインで説明されているように、詳細な説明を書いてください。レビュアーによっては、その製品の機能やコードベースの領域に詳しくないかもしれません。徹底的な説明は、すべてのレビュアーがあなたのリクエストを理解し、効果的にテストするのに役立ちます。
  • もしあなたの変更が、他の変更が先にマージされることに依存していることが分かっている場合は、そのことを説明文に記述し、マージリクエストの依存関係を設定してください。
  • レビュアーからの提案に感謝しましょう。(“Good call. I’ll make that change.”)
  • 個人的に受け取らないでください。レビュアーはコードのレビューであって、あなたのレビューではありません。
  • なぜそのコードが存在するのかを説明してください。(こういう理由でこうなっています。このクラス名/ファイル名/メソッド名/変数名を変更したほうがわかりやすいですか？”)
  • 関係のない変更やリファクタリングを将来のマージリクエスト/イシューに抽出します。
  • レビュアーの視点を理解するように努めましょう。
  • すべてのコメントに答えるようにしましょう。
  • マージリクエスト作成者は、自分が完全にアドレスしたスレッドのみを解決します。未返信、未解決のスレッド、提案、質問、その他がある場合、そのスレッドはレビュアーが解決することに任せるべきです。
  • すべてのフィードバックが、その推奨する変更をマージする前にMRに取り入れることを要求していると考えるべきではないでしょう。それが必要かどうか、または問題の MR がマージされた後、将来そのフィードバックに対応するためにフォローアップイシューを作成すべきかどうかは、MR 作成者とレビュアーが判断することです。
  • 以前のフィードバックに基づくコミットは、ブランチに分離コミットとしてプッシュします。ブランチがマージできるようになるまでは、スクリプトを実行しないでください。レビュアーは、以前のフィードバックに基づく個々の更新を読めるようにしなければなりません。
  • 再レビューの準備ができたら、レビュアーに新しいレビューを依頼してください。レビューをリクエストする機能がない場合は、@ 代わりにレビュアーについて言及してください。

レビューのリクエスト

マージリクエストをレビューする準備ができたら、承認ガイドラインに基づいてレビュアー選択し、初回レビューをリクエストしてください。

マージリクエストに複数のレビュー対象領域がある場合、レビュアーがどの領域のレビューを行うべきか、またどのステージ（1 回目または 2 回目）でレビューを行うべきかを指定することをお勧めします。こうすることで、複数の領域のレビュアーとして資格のあるチームメンバーが、どの領域のレビューを要求されているかを知ることができます。たとえば、マージリクエストにbackend とfrontend の両方の懸念がある場合、レビュアーについてこのように言及することができます:@john_doe can you please review ~backend? または@jane_doe - could you please give this MR a ~frontend maintainer review?

workflow::ready for review ラベルを使うこともできます。これはあなたのマージリクエストがレビューされる準備ができていて、どのレビュアーもそれを選ぶことができることを意味します。時間的なプレッシャーがなく、マージリクエストがレビュアーに割り当てられていることを確認できる場合にのみ、このラベルを使用することを推奨します。

マージリクエストが最初のレビュアーから承認されると、メンテナーに渡すことができます。ドメインの専門知識を持つメンテナーをデフォルトで選び、それ以外はレビュアールーレットの推奨に従うか、ラベルready for merge を使うべきです。

メンテナーがレビューに出られないこともあります。外出中であったり、定員に達している可能性があります。メンテナーのプロフィールで、そのメンテナーがレビュー可能かどうか確認できますし、確認すべきです。ルーレットで推薦されたメンテナーが不在の場合は、そのリストから他の人を選んでください。

マージリクエストをレビューするのは作成者の責任です。ready for review の状態が長すぎる場合は、特定のレビュアーにレビューを依頼することをお勧めします。

レビュアーになるには

GitLab のエンジニアで定員に達している人は、レビューすべきマージリクエストのリストを定期的にチェックし、レビューしたいマージリクエストのレビュアーに追加することができます。

マージリクエストのレビュー

なぜその変更が必要なのかを理解します (バグを修正する、ユーザーエクスペリエンスを向上させる、既存のコードをリファクタリングする)。それから

  • イテレーションの回数を減らすために、徹底したレビュアーになるようにしましょう。
  • あなたが強く感じるアイデアとそうでないアイデアを伝えましょう。
  • 問題を解決しながらコードを簡略化する方法を特定します。
  • 代替の実装を提案しますが、作成者がすでに考慮済みであることを前提にしてください。(「ここでカスタムバリデータを使うのはどうでしょう？」)
  • 作成者の視点を理解するように努めましょう。
  • ブランチをチェックアウトし、変更をローカルでテストします。どの程度の手動テストを行うかは自分で決めましょう。あなたのテストの結果、自動テストを追加する機会が得られるかもしれません。
  • コードの一部が理解できない場合は、_そう_言いましょう。他の誰かが同じように混乱する可能性が高いからです。
  • 作成者がその提案にアドレスし、解決するために何が必要かを明確にするようにしてください。
  • 未解決の依存関係がないことを確認してください。リンクされているイシューにブロッカーがいないか確認してください。必要であれば、作成者に確認してください。1つ以上のオープンMRによってブロックされている場合、MR依存関係を設定します。
  • 一通りのラインノートの後、”Looks good to me “や “Just a couple things to address “などのサマリーノートを投稿すると便利です。
  • レビュー後、変更が必要な場合は作成者に知らせてください。
caution
マージリクエストがフォークからのものである場合、コミュニティ貢献のための追加ガイドラインも確認してください。

マージリクエストのマージ

マージを決定する前に：

  • マイルストーンの設定
  • 正しいMR タイプラベルが適用されていることを確認します。
  • 危険ボット、コード品質、その他のレポーターからの警告やエラーを考慮してください。違反であることを強く主張できない限り、これらはマージする前に解決すべきです。MRが失敗したジョブとマージされた場合、コメントを投稿しなければなりません。
  • MR に品質に関連する変更と品質に関連しない変更の両方が含まれている場合、品質に関連する変更をテスト担当のソフトウェアエンジニアが承認した後、ユーザー向けの変更（バックエンド、フロントエンド、またはデータベース）に関連するメンテナーが MR をマージする必要があります。

MR をマージする前に、少なくとも一人のメンテナーが MR を承認しなければなりません。MR の作成者や、MR にコミットを追加する人は、MR を承認する権限がないため、MR に貢献していないメンテナーに承認を求めなければなりません。一般的には、最終的に必要な承認者が MR をマージする必要があります。

最終承認者がMRをマージしない可能性のあるシナリオ

  • 承認者が、承認後に自動マージを設定するのを忘れた場合。
  • 承認者が、自分が最終承認者であることを認識していません。
  • 承認者が自動マージを設定しているのに、GitLabが未設定にしている場合。

これらのシナリオのいずれかが発生した場合、MR作成者は必要なすべての承認者がいてリポジトリへのマージ権限があれば、自分のMRをマージすることができます。これはまた、GitLabのアクション値のバイアスに沿っています。

このポリシーは、GitLab変更管理コントロールのCHG-04コントロールを満たすために設けられています。

gitlab-org/gitlab でこのポリシーを実装するために、MRがトップレベルのCODEOWNERSメンテナーから承認を得ることを確実にするために、以下の設定を有効にしました：

gitlab-org/gitlab のCODEOWNERS ファイルのコードオーナーを更新するには、コードオーナーの承認ハンドブックのセクションで説明されている手順に従ってください。

ローカルでのリベースや提案の適用など、いくつかのアクションはコミットの追加と同じとみなされ、既存の承認者をリセットする可能性があります。UI からのリベースや/rebase クイックアクションでは承認者は削除されません。

マージの準備ができたら

caution
マージリクエストがフォークからのものである場合、コミュニティ貢献のための追加ガイドラインも確認してください。
  • マージリクエストのコミット数が多い場合、スクワッシュとマージ機能の使用を検討してください。コードをマージするとき、メンテナーは作成者がすでにこのオプションを設定している場合、またはマージリクエストに明らかに乱雑なコミット履歴が含まれている場合のみ、スクワッシュ機能を使うべきです。そうでなければ、MRのコミットが数件しかない場合、作成者の設定を尊重し、コミットをつぶさないようにします。
  • マージリクエストのパイプラインタブに移動し、パイプラインの実行を選択します。そして概要タブで自動マージを有効にします。注意してください：
    • デフォルトブランチが壊れている場合、 非常に特殊な場合を除いてマージリクエストをマージしないでください。それ以外の場合は、ハンドブックの指示に従ってください。
    • マージリクエストが承認される前に最新のパイプラインが作成された場合、新しいパイプラインを開始し、完全な RSpec スイートを実行したことを確認します。マージリクエストにバックエンドの変更が含まれていない場合のみ、この手順を省略できます。
    • 最新のマージ結果パイプラインが 6時間未満前に作成され、2時間未満前に終了した場合、マージリクエストはmainに十分近いため、新しいパイプラインを開始せずにマージすることができます。
  • MRを自動マージに設定すると、それ以降に発見されたリビジョンを引き継ぐ必要があります。
  • Squash と mergeが設定されたマージリクエストでは、つぶされたコミットのデフォルトのコミットメッセージがマージリクエストのタイトルから取得されます。マージする前に、より有益なコミットメッセージを持つコミットを選択することを推奨します。

mainマージ結果パイプラインの**おかげで、作成者はブランチを頻繁にリベースする必要がなくなりました (コンフリクトが発生した場合のみ)。このステップでは、パイプライン作成時に最新のmain に対してマージ結果をテストすることで、実際のマージトレイン機能に近づけます。

コミュニティへの貢献

caution
マージ結果のパイプラインを開始する前に、悪意のあるコードがないかすべての変更を徹底的にレビューしてください。

より広いコミュニティの貢献者によって追加されたマージリクエストをレビューする場合：

  • Ruby gems や Node パッケージなどの新しい依存関係や依存関係の更新に特に注意してください。Gemfile.lock やyarn.lock のようなファイルへの変更は些細なことに見えるかもしれませんが、悪意のあるパッケージの取得につながる可能性があります。
  • 特にドキュメンテーション MR のリンクや画像をレビューしてください。
  • 疑わしい場合は、手動でマージリクエストパイプラインを開始する前に、@gitlab-com/gl-security/appsec の誰かにマージリクエストのレビューを依頼してください。
  • マージリクエストが現在のマイルストーンに含まれそうなときだけ、マイルストーンを設定してください。これは、いつマージされるかという混乱を避けるためと、まだ準備ができていないのにマイルストーンを頻繁に動かすことを避けるためです。

MR ソースブランチがターゲットブランチより 1,000 コミット以上遅れている場合：

  • 作成者にリベースを依頼するか、MR が “Allows commits from members who can merge to the target branch” を有効にしている場合は、bias-for-action を取って自分でリベースすることを検討してください。
  • 最近の変更の文脈で MR をレビュアーすることは、隠れた実行時の衝突を防ぎ、一貫性を促進するのに役立ちます。変更の内容にもよりますが、MR が 1,000 コミット未満であればリベースしたほうがよいでしょう。
  • 無理にプッシュすると投稿者を混乱させる可能性があるため、リベースを行ったことを伝えるか、投稿者がMRをアクティブに作業しているときに最初に確認するのが良いでしょう。
  • リベースは通常、GitLab 内部で/rebase クイックアクションを使って行うことができます。

コミュニティのマージリクエストを引き継ぐ

MRに更なる変更が必要であるにもかかわらず、作成者が長期間応答しない、またはMRを完了できない場合、GitLabはイシューとマージリクエストのクローズポリシーに従ってそれを引き継ぐことができます。GitLabエンジニア（一般的にはマージリクエストのコーチ）が行います：

  1. MRに、マージできるように引き継ぐというコメントを追加します。
  2. 彼らのMRに~"coach will finish" 。
  3. メインブランチから新しいフィーチャーブランチを作成します。
  4. そのブランチを新しい feature ブランチにマージします。
  5. 機能ブランチをメインブランチにマージするために、新しいマージリクエストをオープンします。
  6. あなたの MR からコミュニティ MR をリンクし、~"Community contribution" というラベルをつけてください。
  7. 必要な最終調整を行い、コントリビューターに ping を送り、あなたの変更をレビューする機会を与え、彼らのコンテンツがメインブランチにマージされることを知らせます。
  8. その内容がすべてのマージリクエストガイドラインに準拠していることを確認します。
  9. すべてのマージリクエストに対して行う通常のレビュープロセスに従ってください。

適切なバランス

コードレビューで最も難しいことの一つは、レビュアーが作成者の作成したコードにどれだけ深く干渉できるかの適切なバランスを見つけることです。

  • 正しいバランスを見つける方法を学ぶには時間がかかります。マージリクエストのレビューに時間を費やした後、メンテナーになるレビュアーがいるのはそのためです。
  • バグを見つけることも重要ですが、良いデザインについて考えることも重要です。抽象化と優れたデザインを構築することで、複雑さを隠すことが可能になり、将来の変更が容易になります。
  • コードスタイルの強制と改善は、主にレビューコメントの代わりに自動化によって行われるべきです。
  • 作成者にデザインの変更を依頼することは、貢献するコードを完全に書き直すことを意味することもあります。通常は、それを行う前に他のメンテナーやレビュアーに尋ねるのが良いアイデアですが、重要だと思うときには勇気をもって実行しましょう。
  • イテレーションのために、もしあなたのレビュー提案がノンブロッキングの変更であったり、個人的な好みであったりする場合 (文書化されたり合意された要件ではない)、作成者に戻す前にマージリクエストを承認することを検討してください。こうすることで、作者が同意すればあなたの提案を実装することができますし、メンテナーに渡してすぐにレビューしてもらうこともできます。これにより、マージにかかる時間全体を短縮することができます。
  • 物事を正しく行うことと、今すぐ行うことには違いがあります。理想は前者ですが、現実の世界では後者も必要です。良い例が、できるだけ早くリリースされるべきセキュリティ修正です。緊急の修正であるマージリクエストで、主要なリファクタリングを作成者に依頼することは避けるべきです。
  • 今日うまくやることは、明日完璧にやるよりも良いことです。今日、手抜きのものを出荷することは、明日うまくやるよりも悪いことです。正しいバランスを見つけられないときは、他の人の意見を聞いてください。

GitLab特有の懸念事項

GitLabは様々な場所で使われています。多くのユーザーは私たちのOmnibusパッケージを使っていますが、Dockerイメージを使う人もいますし、ソースからインストールする人もいますし、他のインストール方法もあります。GitLab.com自体は大規模なEnterprise Editionのインスタンスです。これにはいくつかの意味があります：

  1. クエリの変更は、GitLab.comのスケールでパフォーマンスが悪化しないようにテストする必要があります：
    1. ローカルで大量のデータを生成することは有効です。
    2. GitLab.comにクエリプランを求めることが、これらを検証する最も信頼できる方法です。
  2. データベースのマイグレーションには
    1. リバーシブルであること。
    2. GitLab.comの規模では実行可能 - 自信がない場合は、ステージング環境でマイグレーションをテストするようメンテナーに依頼してください。
    3. 正しく分類されています：
  3. Sidekiqワーカーは 後方互換性のない方法で変更することはできません：
    1. Sidekiqのキューはデプロイ前に排出されないため、キューには以前のバージョンのGitLabのワーカーが残っています。
    2. メソッドのシグネチャを変更する必要がある場合は、2つのリリースにまたがって変更し、最初のリリースで古い引数と新しい引数の両方を受け入れるようにしてください。
    3. 同様に、ワーカーを削除する必要がある場合は、あるリリースでスケジュールされないようにし、次のリリースで削除します。これにより、既存のジョブを実行することができます。
    4. すべてのインスタンスがすべての中間バージョンにアップグレードされるわけではないことを忘れないでください（X.1.0からX.10.0へ、あるいはもっと大きなアップグレードを試みる人もいます！）。
  4. キャッシュされた値はリリースをまたいでも残ります。キャッシュ値が返す型を変更する場合 (例えば文字列やnilから配列へ)、キャッシュキーも同時に変更してください。
  5. 設定を追加するのは最後の手段です。GitLab Railsに新しい設定を追加するを参照してください。
  6. クラウドネイティブなアーキテクチャではファイルシステムへのアクセスはできません。実行する必要があるファイルストレージがオブジェクトストレージに対応していることを確認してください。詳細については、アップロードのドキュメントを参照してください。

お客様にとって重要なマージリクエスト

マージリクエストは、そうすることでビジネスに大きな利益があるため、カスタマー クリティカルな優先順位と見なされることがあります。

カスタマー クリティカルなマージリクエストの特性：

  • 開発担当副社長(@clefelhocz1) は、マージリクエストがカスタマー・クリティカルかどうかを決定する承認者です。また、彼の直属のレポーターのうち2人が承認すれば、それも承認者となります。
  • DRI はcustomer-critical-merge-request ラベルをマージリクエストに適用します。
  • カスタマークリティカルなマージリクエストに関わるレビュアーとメンテナーは、この決定がなされるとすぐに参加することが要求されます。
  • カスタマークリティカルなマージリクエストに関わる人たちが、それに集中するために必要な時間を確保できるように、作業の優先順位をつけることが必要です。
  • 顧客にとってクリティカルなマージリクエストに取り組む際には、GitLabの価値観やプロセスを遵守することが求められます。特に、家族や友人が先か仕事が後か、完了の定義、イテレーション、準備が整ったらリリース、といった点に注意してください。
  • カスタマークリティカルなマージリクエストは、技術的な決定の優先順位を決めるプロセスに従って、セキュリティを低下させたり、データ損失のリスクをもたらしたり、可用性を低下させたり、既存の機能を壊したりしないことが求められます。
  • カスタマークリティカルなリクエストでは、効率を犠牲にしてもマージにかかる時間を短縮_できると_考えられる場合、関係者は非同期（マージリクエストコメント）に加えて同期（Zoom、Slack）での調整を_検討する_ことを_推奨します_。
  • カスタマークリティカルなマージリクエストがマージされた後、将来のカスタマークリティカルなマージリクエストの頻度を減らすことを意図して、レトロスペクティブを完了する必要があります。

使用例

コードレビューがどのように行われるかは、新しい貢献者を驚かせることがあります。ここでは、コードレビューの例をいくつか紹介します。

“ModifyDiffNote to reuse it for Designs”： 改行まわりの細かい指摘から、デザインのバージョンとは何かという推論、あるファイルの前のバージョンがない場合にどのように比較すべきか(親 vs. 空白sha vs. 空のツリー)まで、あらゆるコンテナが含まれていました。

“複数行の提案のサポート”:MR自体は、FEとBEのコラボレーションと、レビュアーに対する作成者からのコメントの文書化で構成されています。いくつかの細かい指摘、情報提供のための質問、そして最後にはセキュリティの脆弱性があります。

“プロジェクトごとに複数のリポジトリを許可する”:ZJは他のプロジェクト(Workhorse)に影響を与える可能性について言及し、一貫性を保つためのいくつかの改善を提案しました。また、Jamesのコメントは、全体的なコードの品質（デリゲーションの使用、&. そういったもの）や、コードをより堅牢にすることに役立ちました。

“マージリクエストの複数の担当者をサポート”:コードベースの複数の部分に触れる MR におけるコラボレーションの良い例です。Nickは興味深いエッジケースを指摘し、James Lopezもインポート/エクスポート機能に関する懸念を提起しました。

クレジット

主にthoughtbot コードレビューガイドに基づいています。