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

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

GitLab CEとEEへのマージリクエストは、GitLabチームメンバーによって書かれたものであれ、ボランティアのコントリビューターによって書かれたものであれ、そのコードが効果的で、理解しやすく、メンテナーであることを保証するために、コードレビュープロセスを通らなければなりません。

マージリクエストの確認、承認、マージ

選択したソリューションと実装に対するセカンド・オピニオンを得るため、またバグやロジックの問題、エッジケースの未発見を探すための特別な目を得るために、レビューすべきコードがあればすぐにレビュアーにコードをレビューしてもらうことを強くお勧めします。

デフォルトのアプローチでは、最初のレビューにはあなたのグループまたはチームからレビュアーを選びます。 これはあくまで推奨であり、レビュアーは別のチームであってもかまいません。 しかし、ドメインの専門家である人物を選ぶことを推奨します。

レビュアーが関与することの重要性については、以下の「作成者の責任」のセクションをお読みください。

指導が必要な場合(例えば、初めてのマージリクエストなど)は、お気軽にマージリクエストコーチにお尋ねください。

セキュリティ・スキャンやコメントでサポートが必要な場合は、遠慮なくセキュリティ・チーム (@gitlab-com/gl-security) をレビュアーに加えてください。

マージリクエストの対象領域によっては、1人以上のメンテナーの承認が必要です:

承認については、マージリクエストウィジェットにある承認機能を使用します。 レビュアーは追加承認することで承認を追加できます。

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

ドメインの専門家

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

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

私たちは、自動的にドメインの専門家と見なされることに関して、次のような前提を置いています:

  • 特定のステージ/グループ(例:ソースコードの作成)で作業するチームメンバーは、担当するアプリのその分野のドメインエキスパートとみなされます。
  • 特定の機能(検索など)に取り組んでいるチームメンバーは、その機能のドメインエキスパートとみなされます。

適切な専門家がいない場合は、どのチームメンバーでもMRをレビューすることができますし、レビュアー・ルーレットの推奨に従ってください。

チームメンバーのドメインの専門知識は、エンジニアリングプロジェクトのページやGitLabチームのページで見ることができます。

レビュアー ルーレット

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

レビュアーとメンテナーは、エンジニアリング・プロジェクトのページにあるリストから選びます:

  1. GitLabのステータスに‘OOO’という文字列が含まれている人や、絵文字が:palm_tree::beach:の人は選ばれません。
  2. 研修生メンテナーは、他のレビュアーに比べて3倍の確率で選ばれます。
  3. 常に同じブランチ名の同じレビュアーとメンテナーを選びます (ポイント 1 のように OOO のステータスが変わらない限り)。 バックポートブランチでも安定するように、先頭のce-ee-、末尾の-ce-eeを削除します。

承認者ガイドライン

以下の「メンテナーの責任」のセクションで述べられているように、あなたのマージリクエストは、ドメインの専門知識を持つメンテナーに承認してもらい、マージしてもらうことをお勧めします。

  1. あなたのマージリクエストがバックエンドの変更を含む場合(1)、バックエンドメンテナーの承認が必要です。
  2. マージリクエストにデータベースの移行や高価なクエリの変更が含まれる場合(2)、データベースメンテナーの承認が必要です。 詳細はデータベースレビューガイドラインをお読みください。
  3. マージリクエストがフロントエンドの変更を含む場合(1)、フロントエンドメンテナーの承認が必要です。
  4. マージリクエストにUXの変更(1)が含まれる場合、UXチームメンバーの承認が必要です。
  5. マージリクエストに新しいJavaScriptライブラリ(1)の追加が含まれる場合、フロントエンドのリードによる承認が必要です。
  6. マージリクエストに新しいUI/UXパラダイム(1)の追加が含まれる場合は、UXリードの承認が必要です。
  7. マージリクエストに新しい依存関係やファイルシステムの変更が含まれる場合は、ディストリビューションチームメンバーの承認が必要です。 詳細については、ディストリビューションチームとの連携方法をご覧ください。
  8. マージリクエストに文書の変更が含まれる場合は、該当する製品カテゴリーに基づき、テクニカルライターの承認が必要です。
  9. マージリクエストにエンドツーエンドおよび非エンドツーエンドの変更(3)が含まれる場合、テスト担当のソフトウェアエンジニアによる承認が必要です。
  10. マージリクエストにエンドツーエンドの変更(3)のみが含まれる場合、またはMR作成者がテスト担当のソフトウェアエンジニアである場合、品質メンテナーの承認が必要です。
  • (1):JavaScript以外の仕様はバックエンドコードとみなされますのでご注意ください。
  • (2): マージ・リクエストに高価なクエリを使用する可能性がある場合は、データベースのメンテナーに指導を求めることをお勧めします。 SQLクエリとともに問題のコード行にコメントするのが最も効率的です。
  • (3): エンドツーエンドの変更には、qa ディレクトリ内のすべてのファイルが含まれます。

セキュリティ要件

いつどのようにセキュリティレビューを依頼するかについては、内部アプリケーションセキュリティレビューに関する最新の文書をご覧ください。

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

最善の解決策を見つけ、それを実行する責任はマージリクエスト作成者にあります。

承認とマージのためにマージリクエストをメンテナーに割り当てる前に、メンテナーは次のことを確信すべきです:

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

これを行う最良の方法は、レビュアーとの不必要なやり取りを避けるために、コードレビューガイドラインに従ってマージリクエストのセルフレビューを行うことです。

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

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

作成者がマージリクエストにドメインエキスパートの意見が必要かどうか確信が持てない場合、それは通常、必要であることを示す良いサインです。

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

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

避けてください:

  • レビュアーからの要求がない限り、ソースコードに直接コメント(上記参照、またはTODO項目)を追加してください。 実行可能なタスクのためにコメントを追加する場合は、イシューへのリンクを含める必要があります。
  • テストが失敗したマージリクエストをメンテナーに割り当てます。 テストが失敗して割り当てなければならない場合は、必ずコメントで説明を残してください。
  • メールやSlackでメンテナーに過度に言及する (メンテナーがSlackで連絡可能な場合)。 マージリクエストを割り当てることができない場合、@ コメントでメンテナーに言及することは許容され、それ以外の場合はマージリクエストを割り当てることで十分です。

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

レビュアーの責任

マージリクエストを十分にレビュアーし、すべての要件を満たしていることが確認できたら、マージリクエストを行ってください:

  • 承認者ボタンをクリックします。
  • マージリクエストがレビューされ、承認されたことを作成者に通知します。
  • マージリクエストをメンテナーに割り当てます。 デフォルトではドメインの専門知識を持つメンテナーに割り当てますが、メンテナーがいない場合や、マージリクエストにドメインの専門家によるレビュアーが必要ないと考える場合は、レビュアールーレットの提案に従ってください。

メンテナーの責任

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

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

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

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

レビュアーは、MR がドメイン・エキスパートのレビューが必要なほど重要であり、ドメイン・ エキスパートがこれまでのレビューに関与しているかどうか不明な場合、MR をマージする前にドメイン・エキスパートにレビューをリクエストすることができます。

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

メンテナーはマージする前に、そのマージリクエストが必要な承認者によって承認されているかどうかを確認する必要があります。

メンテナーはマージリクエストの前に、マージリクエストセキュリティウィジェットのリストを検査することによって、マージリクエストが新しい脆弱性を導入しているかどうかをチェックしなければなりません。 疑わしい場合は、セキュリティエンジニアが関与することができます。 検出された脆弱性のリストは空であるか、含まれていなければなりません:

  • 誤検知時の脆弱性を排除
  • 脆弱性がイシューに転換

メンテナーは、脆弱性をきちんと検証することなく、リストを “空っぽ “にしてしまうようなことがあってはなりません。

ある種のマージリクエストは安定版ブランチを対象とする場合があることに注意してください。 これはまれなイベントです。 このようなタイプのマージリクエストはメンテナーではマージできません。 代わりに、これらはリリースマネージャに送ってください。

ベストプラクティス

みんな

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

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

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

  • あなたのコードの最初のレビュアーは_あなた_です。 ピカピカの新しいブランチを最初にプッシュする前に、差分全体に目を通してください。 意味のあるものになっていますか? 変更の全体的な目的とは関係のないものが含まれていませんか? デバッグ用のコードを削除し忘れていませんか?
  • レビュアーからの提案に感謝しましょう。
  • レビュアーはあなたではなく、コードについてです。
  • なぜそのようなコードが存在するのかを説明しなさい。 このクラス/ファイル/メソッド/変数の名前を変えたら、もっとわかりやすくなりますか?
  • 関係のない変更やリファクタリングを将来のマージリクエスト/イシューに抽出します。
  • レビュアーの視点を理解するように努めましょう。
  • すべてのコメントに返信するようにしてください。
  • マージリクエスト作成者は、自分が完全に対処したスレッドのみを解決します。 未解決の返信、未解決のスレッド、提案、質問、その他がある場合、そのスレッドはレビュアーが解決することに任せるべきです。
  • すべてのフィードバックが、その推奨される変更をマージ前にMRに取り込む必要があると考えるべきではないでしょう。 これが必要かどうか、または問題のMRがマージされた後、将来そのフィードバックに対応するためにフォローアップイシューを作成すべきかどうかは、MRの作成者とレビュアーが判断することです。
  • 以前のフィードバックに基づくコミットは、ブランチに分離したコミットとしてプッシュします。 ブランチがマージできる状態になるまで、スクワッシュはしないでください。 レビュアーは、以前のフィードバックに基づく個々の更新を読むことができるはずです。
  • 再レビューの準備ができたら、マージリクエストをレビュアーに割り当ててください。マージリクエストを割り当てる機能がない場合は、@代わりにレビュアーに言及してください。

レビュアーへのマージリクエストの割り当て

マージリクエストをレビューする準備ができたら、最初のレビューではあなたのグループまたはチームのレビュアーに割り当てるのがデフォルトですが、任意のレビュアーに割り当てることもできます。 レビュアーのリストはエンジニアリングプロジェクトページで見ることができます。

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

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

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

レビュアーが準備できたマージリクエストのリスト

能力のある開発者は、レビューするマージリクエストリストを定期的にチェックし、レビューしたいマージリクエストを割り当てることができます。

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

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

  • 繰り返しの回数を減らすために、レビュアーは徹底するようにしてください。
  • あなたが強く感じているアイデアとそうでないアイデアを伝えてください。
  • 問題を解決しながらコードを簡略化する方法を特定します。
  • 代替の実装を提案しますが、作成者がすでに考慮済みであることを前提とします。
  • 作成者の視点を理解すること。
  • もしコードの一部が理解できないなら、_そう言って_ください。 他の誰かが同じように混乱する可能性があります。
  • 作成者が、その提案に対処/解決するために何が必要かを明確にするようにしてください。
    • あなたの意図を伝えるために、従来のコメント形式を使用することを検討してください。
    • 必須でない提案については、(non-blocking) で飾り、作成者がマージリクエストの中で解決するか、後のステージでフォローアップできるようにします。
  • ラインノートを一巡した後、”Looks good to me “や “Just a couple things to address “といったサマリーノートを投稿すると便利です。
  • レビュアーによるレビュー後に変更が必要な場合は、作成者にマージリクエストを割り当ててください。

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

マージに踏み切る前に:

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

マージリクエストの基本的な準備はできているが、些細な修正 (誤字脱字など) だけが必要な場合、作成者に戻ることなく直接変更を加えることで、行動の偏りを示すことを検討してください。 これは、マージリクエストにあなた自身の提案を適用するために、変更を提案する機能を使うことで行うことができます。 以下のことに注意してください:

  • 変更が簡単でない場合、作成者にマージリクエストを割り当てることを優先してください。
  • 提案を適用する前に、マージリクエストを編集し、squash and mergeが有効になっていることを確認してください。そうでない場合、パイプラインのDangerジョブは失敗します。
    • マージリクエストでsquashとmergeが有効になっておらず、複数のコミットがある場合、コミット履歴の書き換えについては以下の注意を参照してください。

マージする準備ができたら:

  • マージリクエストにたくさんのコミットがある場合は、スクワッシュとマージ機能を使うことを検討してください。 コードをマージするときにメンテナーがスクワッシュ機能を使うのは、作成者がすでにこのオプションを設定しているか、マージリクエストに明らかにスクワッシュを意図した乱雑なコミット履歴がある場合だけにしてください。
  • マージリクエストの “パイプライン “タブにあるRun Pipeline ボタンで新しいマージリクエストパイプラインを開始し、”パイプライン成功時にマージ “を有効にします(MWPS). 注意してください:
    • 最新のマージ結果のパイプライン が2時間以内に終了した場合、マージリクエストはmasterに十分近いため、新しいパイプラインを開始せずにマージすることができます。
    • マージリクエストがフォークからのものである場合、マージ結果のパイプラインを使うことができません。そのため、masterを壊す可能性が高くなります。 ソースブランチがmaster からどれくらい遅れているかをチェックしてください。100コミット以上遅れている場合は、マージする前に作成者にリベースするよう依頼してください。
    • masterが壊れている場合、上記の2つのルールに加えて、master 、赤い “マージ “ボタンをクリックする前に~”master:broken “イシューへのリンクを投稿してください。
  • MRを “パイプラインが成功したらマージ “に設定した場合、それ以降に発見されたリビジョンを引き継ぐ必要があります。
注:“Pipeline for Merged Results” のおかげで、作成者はブランチを頻繁にリベースする必要がなくなりました (コンフリクトが発生した場合のみ)。 マージ結果パイプラインにはすでに.MWPS **の最新の変更が反映されているからです。この結果 、メンテナーは最終的なリベースを依頼する必要がなくなり、MRパイプラインを開始して MWPS を設定するだけでよいので 、レビュ/マージサイクルが速くなります。 このステップでは 、パイプライン作成時にマージ結果を最新と比較するテストを行うことで、実際のマージトレイン **機能に近づけます。

適切なバランス

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

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

GitLab特有の懸念事項

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

  1. クエリの変更 、GitLab.comのスケールでパフォーマンスが悪化しないかテストする必要があります:
    1. 現地で大量のデータを生成することは有効です。
    2. GitLab.comにクエリプランを問い合わせるのが、クエリプランを検証する最も信頼できる方法です。
  2. データベース移行
    1. リバーシブル。
    2. GitLab.comのような規模では実行可能です。もし自信がなければ、ステージング環境でマイグレーションをテストするようメンテナーに頼んでください。
    3. 正しく分類されています:
      • 通常のマイグレーションは、新しいコードがインスタンス上で実行される前に実行されます。
      • デプロイ後のマイグレーションは、新しいコードがデプロイされた_後_、インスタンスがそのように設定されている場合に実行されます。
      • バックグラウンドマイグレーションはSidekiqで実行され、GitLab.comの規模では極端に時間がかかるマイグレーションにのみ行うべきです。
  3. Sidekiqのワーカー後方互換性のない方法で変更することはできません
    1. Sidekiqのキューはデプロイ前に排出されないので、前のバージョンのGitLabのワーカーがキューに残ります。
    2. メソッドのシグネチャを変更する必要がある場合は、2つのリリースにまたがって変更し、最初のリリースで古い引数と新しい引数の両方を受け入れるようにしてください。
    3. 同様に、ワーカーを削除する必要がある場合は、あるリリースでスケジュールされないようにし、次のリリースで削除します。 これにより、既存のジョブを実行できるようになります。
    4. すべてのインスタンスがすべての中間バージョンにアップグレードするわけではない(X.1.0からX.10.0にアップグレードする人もいるでしょうし、もっと大きなアップグレードに挑戦する人もいるでしょう!)ことを忘れないでください。
  4. キャッシュされた値 は、リリースをまたいで持続する可能性があります。 キャッシュされた値が返す型を変更する場合(たとえば、文字列やnilから配列へ)、キャッシュ・キーも同時に変更してください。
  5. 設定最後の手段として追加してください。gitlab.ymlで新しい設定を追加する場合:
    1. そうならないように、ApplicationSetting
    2. オムニバスにも追加されていることを確認してください。
  6. ファイルシステムへのアクセスは遅くなることがあるので、代替手段がある場合は共有ファイルを避けるようにしてください。

レビュアー納期

他の人のブロックを解除することは常に最優先事項であるため、レビュアーは、他のタスクや優先事項に悪影響を及ぼす可能性がある場合でも、割り当てられたマージリクエストをタイムリーにレビューすることが求められます。

そうすることで、マージリクエストに関わるすべての人が、文脈が記憶に新しいうちに、より速く反復できるようになり、貢献者の経験値が大幅に向上します。

レビュアーSLO

すぐにレビューできるコードへの迅速なフィードバックを保証するために、私たちはReview-response サービスレベル目標(SLO)をメンテナーとしています。 SLO は次のように定義されています:

  • レビュー回答SLO=(最初のレビュー回答が提供された時間)-(MRがレビュアーに割り当てられた時間)<2営業日

もしあなたがマージリクエストをReview-response SLOの時間内にレビューできないと思われる場合、できるだけ早く作成者にその旨を伝え、できる他のレビュアーやメンテナーを見つける手助けをして、ブロックを解除して迅速に作業に取り掛かれるようにしてください。

定員に達しており、いくつかのレビューが完了するまでこれ以上レビューを受け付けることができない場合は、GitLabのステータスでそのことを伝えましょう。:red_circle: の絵文字を設定し、ステータステキストに定員に達していることを明記してください。そうすることで、投稿者が別のレビュワーを選ぶように誘導し、SLOを達成できるようになります。

もちろん、あなたが不在でGitLab.comのステータスを介してこのことを伝えている場合、作成者はこのことを理解し、自分で別のレビュアーを見つけてください。

マージリクエスト作成者がReview-response SLOよりも長くブロックされている場合、Slackを通じてレビュアーにリマインドするか、別のレビュアーを割り当てることができます。

顧客の重要なマージリクエスト

マージリクエストは、顧客にとって重要な優先事項であると見なされることで、ビジネスに大きな利益をもたらす可能性があります。

カスタマークリティカルなマージリクエストのプロパティ:

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

使用例

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

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

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

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

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

クレジット

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


開発者のドキュメントに戻る