Code Review Guidelines

Code Review Guidelines

本指南包含有关执行代码审查和代码审查的建议和最佳实践.

无论是由 GitLab 团队成员还是志愿者贡献者编写的所有对 GitLab CE 和 EE 的合并请求，都必须经过代码审查流程，以确保代码有效，可理解，可维护和安全.

强烈建议您让您的代码通过审核 ，一旦有任何代码评审，获得所选择的解决方案和实施，和一个额外的一双眼睛寻找错误，逻辑问题，或裸露边缘的第二意见案件.

默认方法是从您的小组或团队中选择一名审阅者进行第一次审阅. 这只是一个建议，审阅者可能来自其他团队. 但是，建议选择一个领域专家 .

您可以在下面有关作者责任的部分中详细了解让审稿人参与的重要性.

如果您需要一些指导（例如，这是您的第一个合并请求），请随时咨询一位 .

如果您需要有关安全扫描或注释的帮助，请随时在评论中包括安全团队（ ）.

根据您的合并请求涉及的领域，它必须由一个或多个维护者 批准 ：

对于批准，我们使用合并请求小部件中的批准功能. 审阅者可以通过批准来添加其批准.

让您的合并请求也合并需要一个维护者. 如果需要多个批准，则最后一个对其进行审核的维护者也会将其合并.

领域专家是在特定技术，产品功能或代码库领域具有丰富经验的团队成员. 鼓励团队成员自认是领域专家，并将其添加到他们的团队资料中

当自我确定为领域专家时，建议分配更改team.yml的 MR，以由已经建立的领域专家或相应的工程经理进行合并.

对于自动被视为领域专家，我们做出以下假设：

  • 在特定阶段/小组中工作的团队成员（例如，创建：源代码）被视为他们所从事的应用程序领域的领域专家
  • 从事特定功能（例如搜索）的团队成员被视为该功能的领域专家

我们默认为具有领域专业知识的团队成员分配评论. 如果没有合适的 ，您可以选择任何团队成员来审核 MR，也可以按照” 审核者”轮盘赌的建议进行操作.

可以在页面或GitLab 团队页面上查看团队成员的领域专业知识.

Reviewer roulette

Danger 机器人会为您的合并请求似乎接触的每个代码库区域随机选择一个审阅者和一个维护者. 它仅提出建议 ，如果您认为其他人更适合，则应予以覆盖！

它从页面的列表中选择审阅者和维护者，具有以下行为：

  1. 它不会选择GitLab 状态包含字符串’OOO’或表情符号为:palm_tree:或:beach: .
  2. 被选拔的可能性是其他审阅者的三倍.
  3. 它始终为相同的分支名称选择相同的审阅者和维护者（除非他们的 OOO 状态更改，如第 1 点所示）. 它消除导致ce-和ee-和尾-ce和-ee ，以便它可以为反向移植分支稳定.

Approval guidelines

如下面有关维护者职责的部分所述，建议您让合并请求由具有域专业知识的维护者批准并合并.

  1. 如果您的合并请求包含后端更改（ 1 ），则必须由批准 .
  2. 如果您的合并请求包括数据库迁移或对昂贵查询的更改（ 2 ），则必须得到数据库维护者的批准 . 阅读以获取更多详细信息.
  3. 如果您的合并请求包含前端更改（ 1 ），则必须得到前端维护者的批准 .
  4. 如果您的合并请求包括添加新的 JavaScript 库（ 1 ），则必须得到批准 .
  5. 如果您的合并请求包括添加新的 UI / UX 范例（ 1 ），则必须由UX 主管批准 .
  6. 如果您的合并请求包含新的依赖项或文件系统更改，则必须得到批准 . 有关更多详细信息，请参见如何与发行团队合作.
  7. 如果您的合并请求中包含文档更改，则必须根据相应的 由技术撰稿人批准 .
  8. 如果您的合并请求包含端到端和非端到端更改（ 3 ），则必须经过的软件工程师的批准 .
  9. 如果您的合并请求仅包含端到端更改（ 3 ）， 或者如果 MR 作者是的软件工程师 ，则必须得到批准
  • （ 1 ）：请注意，除 JavaScript 规范以外的其他规范均被视为后端代码.
  • （ 2 ）：如果您的合并请求可能引入昂贵的查询，我们鼓励您从数据库维护者那里寻求指导. 用 SQL 查询在相关代码行中注释是最有效的，这样他们就可以给出建议.
  • （ 3 ）：端到端更改包括qa目录中的所有文件.

Security requirements

View the updated documentation regarding for when and how to request a security review.

The responsibility of the merge request author

找到最佳解决方案并实施该解决方案的责任在于合并请求作者.

在将合并请求分配给维护者进行批准和合并之前，他们应该确信：

  • 它实际上解决了本应解决的问题.
  • 它以最合适的方式这样做.
  • 满足所有要求.
  • 没有剩余的错误，逻辑问题，未发现的极端情况或已知的漏洞.

做到这一点并避免与审阅者不必要的来回交流的最佳方法是，按照指南对自己的合并请求进行自审.

鼓励他们与领域专家联系 ，讨论不同的解决方案或对实现进行审查，与产品经理和 UX 设计师联系，以消除混乱或验证最终结果是否与他们的想法相符，并与数据库专家联系，以获取有关数据模型或特定查询，或者让任何其他开发人员深入了解该解决方案.

如果作者不确定合并请求是否需要意见，这通常是一个很好的信号，因为如果没有合并请求，将无法达到他们对解决方案所要求的置信度.

在审阅之前，请作者针对合并请求差异提交评论，以提醒审阅者重要的事项以及需要进一步解释或关注的事项. 可能需要发表评论的内容示例包括：

  • 增加了起毛规则（Rubocop，JS 等）.
  • 增加了一个库（Ruby gem，JS lib 等）.
  • 如果不明显，则指向父类或方法的链接.
  • 进行任何基准测试以补充变更.
  • 潜在的不安全代码.

Avoid:

  • 除非审阅者要求您将注释（上面引用的或 TODO 项）直接添加到源代码中. 如果由于可执行的任务而添加了注释，则必须包含指向问题的链接.
  • 将测试失败的合并请求分配给维护人员. 如果测试失败，则必须分配，请确保在评论中留下说明.
  • 通过电子邮件或 Slack 过多提及维护者（如果可以通过 Slack 与维护者联系）. 如果您不能分配合并请求，则@可以在注释中提及维护者，在所有其他情况下，分配合并请求就足够了.

This saves reviewers time and helps authors catch mistakes earlier.

The responsibility of the reviewer

彻底检查合并请求 . 当您确信它满足所有要求时，您应该：

  • Click the Approve button.
  • 告知作者他们的合并请求已经过审核和批准.
  • 将合并请求分配给维护者. 默认情况下，将其分配给具有的维护者，但是，如果不可用，或者您认为合并请求不需要领域专家的审查，请随时遵循” 建议.

维护人员负责跨领域和产品领域的 GitLab 代码库的总体运行状况，质量和一致性.

因此，他们的审查将主要集中在总体架构，代码组织，关注点分离，测试，DRYness，一致性和可读性等方面.

由于维护人员的工作仅取决于他们对整个 GitLab 代码库的了解，而不取决于任何特定领域的知识，因此他们可以查看，批准和合并来自任何团队和任何产品领域的合并请求.

维护人员将尽最大努力在合并之前检查所选解决方案的详细信息，但是由于他们不一定是领域专家 ，因此他们可能没有太多时间来浪费很多时间. 在这种情况下，他们将服从作者和早期审稿人的判断，而将精力放在他们的主要职责上.

如果维护者认为 MR 足够重要，足以保证需要由进行审查，并且目前尚不清楚域专家是否参与了审查，则他们可以在合并 MR 之前要求域专家进行审查.

如果恰好也是维护者的开发人员作为审阅者参与了合并请求，则建议不要同时选择他们作为维护者来最终批准并合并它.

维护人员应在合并之前检查合并请求是否已被所需批准者批准.

维护人员必须在合并之前检查合并请求的列表，以检查合并请求是否引入了新的漏洞. 如有疑问，可以请安全工程师参与. 检测到的漏洞列表必须为空或包含以下内容：

  • 在出现误报的情况下消除漏洞
  • 漏洞转化为问题

维护人员切勿在没有适当验证的情况下消除漏洞以”清空”列表.

请注意，某些合并请求可能会针对稳定分支. 这些是罕见的事件. 维护者无法合并这些类型的合并请求. 相反，这些应该发送到 .

Everyone

  • 善待.
  • 接受许多编程决策是意见. 讨论您喜欢的折衷方案，并迅速解决.
  • 问问题; 不要提出要求. （”您如何命名这个 ？”）
  • 要求澄清. （”我听不懂.您能澄清一下吗？”）
  • 避免有选择地拥有代码. （”我的”，”不是我的”，”您的”）
  • 避免使用可能被视为涉及个人特质的术语. （”哑巴”，”愚蠢”）. 假设每个人都是有吸引力，聪明和善良的.
  • 要明确. 请记住，人们并不总是在线了解您的意图.
  • 要谦虚. （”我不确定-让我们看一下.”）
  • 不要夸张. （”始终”，”从不”，”无限”，”无”）
  • 使用讽刺时要小心. 我们所做的一切都是公开的； 对于您和长期的同事而言，似乎很老套的话可能会很卑鄙，不欢迎新加入该项目的人.
  • 如果”我听不懂”或”替代解决方案：”注释过多，请考虑一对一聊天或视频通话. 发表后续评论，总结一对一的讨论.
  • 如果您向特定人员提出问题，请务必先提及他们，然后再开始评论； 如果将通知级别设置为”提及”，这将确保他们看到该消息，并且其他人将理解他们不必响应.

Having your merge request reviewed

请记住，代码审阅是一个可能需要多次迭代的过程，审阅者可能会在以后发现他们第一次看不到的东西.

  • 您的代码的第一位审阅者是you . 在对闪亮的新分支进行第一次推送之前，请通读整个差异. 是否有意义？ 您是否包含与变更的总体目的无关的内容？ 您是否忘记删除任何调试代码？
  • 感谢审阅者的建议. （”好电话.我会进行更改.”）
  • 不要亲自去做. 审阅的是代码，而不是您的.
  • 说明代码为何存在. （”由于这些原因就这样.如果重命名这个类/文件/方法/变量，是否更清楚？”）
  • 将不相关的更改和重构提取到将来的合并请求/问题中.
  • 试图了解审稿人的观点.
  • 尝试回应每个评论.
  • 合并请求作者仅解析他们已完全解决的线程. 如果有公开的答复，公开的话题，建议，问题或其他任何内容，则该话题应留待审阅者解决.
  • 不应假定所有反馈都要求在合并之前将其建议的更改合并到 MR 中. 这是 MR 作者和审阅者对是否需要这样做的判断，或者是在合并有问题的 MR 之后是否应创建后续问题以解决将来的反馈.
  • 基于较早回馈的推送提交作为对分支的独立提交. 在分支准备好合并之前，请勿压扁. 审阅者应该能够根据他们先前的反馈来阅读各个更新.
  • 准备好进行另一轮审核后，将合并请求分配回审核者. 如果您无法分配合并请求，请@提及审阅者.

Assigning a merge request for a review

准备好要审核合并请求时，默认应将其分配给小组或团队中的审阅者以进行第一次审阅，但是，您也可以将其分配给任何审阅者. 审阅者列表可以在” 页面上找到.

You can also use workflow::ready for review label. That means that your merge request is ready to be reviewed and any reviewer can pick it. It is recommended to use that label only if there isn’t time pressure and make sure the merge request is assigned to a reviewer.

在审核合并请求并将其传递给维护者后，您应该默认选择具有领域专业知识的维护者，否则应遵循 Reviewer Roulette 的建议或使用ready for merge的标签.

合并请求的作者有责任审查合并请求. 如果ready for review状态太长时间，建议将其分配给特定的审阅者.

List of merge requests ready for review

有能力的开发人员可以定期检查合并请求列表并分配他们要审核的任何合并请求.

Reviewing a merge request

  • 尝试在您的评论中做到周密，以减少迭代次数.
  • 交流您对哪些想法有强烈的想法，而哪些想法则没有.
  • 确定在解决问题的同时简化代码的方法.
  • 提供替代的实现，但是假设作者已经考虑了它们. （”您在这里对使用自定义验证器有何看法？”）
  • 如果您不懂一段代码，请这样说 . 很有可能其他人也会对此感到困惑.
  • 确保作者清楚他们需要什么来解决/解决该建议.
    • 考虑使用来传达您的意图.
    • 对于非强制性建议，请使用（非阻塞）修饰，以便作者知道可以选择在合并请求中解决该问题，或者在以后进行后续操作.
  • 经过几行注释后，发布摘要注释（例如”对我很好”或”仅几件要解决的问题”）会很有帮助.
  • 如果审阅后需要更改，则将合并请求分配给作者.

在决定合并之前：

  • 设置里程碑.
  • 考虑来自危险漫游器，代码质量和其他报告的警告和错误. 除非有充分的理由证明违规，否则应在合并前解决这些问题. 如果 MR 与任何失败的作业合并，则必须发布评论.
  • 如果 MR 既包含与质量相关的变更，又包含与非质量无关的变更，则应由相关维护人员合并 MR，以便在测试中软件工程师批准与质量相关的变更后，将其面向用户的变更（后端，前端或数据库）.

如果合并请求从根本上准备就绪，但仅需要简单的修正（例如拼写错误），则可以考虑通过直接进行更改而不向作者证明是对行动的 . 您可以通过使用” 建议更改”功能将自己的建议应用于合并请求来实现. 注意：

  • 如果更改不直接，请优先将合并请求分配回作者.
  • 在应用建议之前 ，请编辑合并请求以确保启用了 ，否则，管道的”危险”作业将失败.
    • 如果合并请求未启用压缩和合并，并且具有多个提交，则请参阅以下有关重写提交历史记录的注释.

准备合并时：

  • 当合并请求包含大量提交时，请考虑使用Squash 和合并功能. 合并代码时，维护者仅应在作者已设置此选项或合并请求中明确包含要压缩的混乱提交历史记录时使用 squash 功能.
  • 使用合并请求的”管道”选项卡中的” Run Pipeline按钮启动新的合并请求管道，并启用”管道成功时合并”（MWPS）. 注意：
    • 如果最新不到 2 小时前完成，则由于合并请求足够接近master ，您可能无需启动新管道就可以合并.
    • 如果合并请求来自 fork ，则我们不能将管道用于合并结果 ，因此，它们更容易破坏master . 检查源分支在master . 如果超过 100 次提交，请在合并之前要求作者重新设置基础.
    • 如果 ，除了上述两个规则外，请检查master是否也发生任何故障，并在单击红色的” Merge”（合并）按钮之前发布指向〜” master：broken”问题的链接.
  • 当将 MR 设置为”管道成功合并时”时，您应该接管后续修订，以应对之后发现的所有问题.

注意：感谢”合并结果管道”，由于合并结果管道已经合并了的最新更改，因此作者不必再频繁地重新建立分支基础（仅当发生冲突时）. 由于维护人员不必要求最终的重新定基，因此可以加快审核/合并周期：相反，他们只需要启动 MR 管道并设置 MWPS. 通过在创建管道时针对最新master测试合并结果，此步骤使我们非常接近实际的合并训练功能.

The right balance

在代码审查期间，最困难的事情之一是在审查者可以干扰作者创建的代码的深度上找到适当的平衡.

  • 学习如何找到合适的平衡点需要花费时间. 这就是为什么我们花了一些时间审核合并请求后使审核员成为维护者的原因.
  • 查找错误很重要，但是考虑良好的设计也很重要. 建立抽象和良好的设计可以隐藏复杂性并使将来的更改变得容易.
  • 强制和改进应主要通过自动化而不是评论注释来完成.
  • 要求作者更改设计有时意味着完全重写贡献的代码. 通常，在进行此操作之前先询问其他维护者或审阅者是一个好主意，但是当您认为重要时，要有勇气去做.
  • 为了的利益，如果您的审查建议是非阻塞性更改或个人喜好（不是书面或约定的要求），请考虑批准合并请求，然后再将其传递回作者. 如果他们同意，这将使他们能够实施您的建议，或者使他们立即将其传递给维护者以供审核. 这可以帮助减少我们的整体合并时间.
  • 做正确的事和立即做事有区别. 理想情况下，我们应该做前者，但在现实世界中，我们也需要后者. 一个很好的例子是安全修复程序，应尽快发布. 应该避免要求作者在合并请求中进行重大重构，这是一个紧急解决方案.
  • 今天做好事通常比明天做好事更好. 今天运送垃圾通常比明天做好事更糟. 当您找不到合适的平衡时，请向其他人询问他们的意见.

GitLab-specific concerns

GitLab 在很多地方都使用过. 许多用户使用我们的 ，但有些用户使用Docker 映像 ，有些是 ，还有其他可用的安装方法. GitLab.com 本身是一个大型企业版实例. 这有一些含义：

  1. 应该对查询更改进行测试，以确保在 GitLab.com 规模上不会导致性能下降：
    1. 在本地生成大量数据会有所帮助.
    2. 从 GitLab.com 询问查询计划是验证这些计划的最可靠方法.
  2. 数据库迁移必须是：
    1. 可逆的.
    2. 性能达到 GitLab.com 的规模-如果不确定，请维护人员在登台环境中测试迁移.
    3. 正确分类：
      • 常规迁移在新代码在实例上运行之前运行.
      • 将实例配置为执行新部署 后 ，便会进行部署后 .
      • 后台迁移在 Sidekiq 中运行，并且仅应在 GitLab.com 规模上花费大量时间进行迁移.
  3. Sidekiq 工人 :
    1. 在部署发生之前，不会耗尽 Sidekiq 队列，因此队列中会有来自上一版 GitLab 的工作线程.
    2. 如果需要更改方法签名，请尝试在两个版本中进行更改，并在第一个版本中同时接受新旧参数.
    3. 同样，如果您需要删除一个工作程序，请停止在一个版本中计划它，然后在下一个版本中将其删除. 这将允许现有作业执行.
    4. Don’t forget, not every instance will upgrade to every intermediate version (some people may go from X.1.0 to X.10.0, or even try bigger upgrades!), so try to be liberal in accepting the old format if it is cheap to do so.
  4. 缓存的值可能会在各个发行版中持续存在. 如果要更改缓存值返回的类型（例如，从字符串或 nil 到数组），请同时更改缓存键.
  5. 设置应作为最后的手段添加. 如果要在gitlab.yml添加新设置：
    1. 尝试避免这种情况，而是添加到ApplicationSetting .
    2. 确保它也已 .
  6. 文件系统访问可能很慢，因此，在有替代解决方案可用时，请尝试避免共享文件 .

Review turnaround time

由于取消阻止其他人始终是头等大事 ，因此，即使这可能会对他们的其他任务和优先事项产生不利影响，审阅者也应及时查看分配的合并请求.

这样做使合并请求中涉及的每个人都可以在上下文处于内存中新鲜时更快地进行迭代，并显着改善了贡献者的体验.

Review-response SLO

为了确保快速反馈到准备就绪的代码，我们维护了一个Review-response服务级别目标（SLO）. SLO 定义为：

  • 审核响应 SLO =（提供第一次审核响应的时间）-（将 MR 分配给审核者的时间）<2 个工作日

如果您认为您无法在” Review-response SLO 期限内审阅合并请求，请让作者尽快知道，并尝试帮助他们找到其他能够审阅的审阅者或维护者，因此他们可以畅通无阻，并迅速开始工作.

如果您认为自己有能力并且在完成一些评论之前无法接受其他评论，请通过设置:red_circle:表情符号并在状态文本中提及您有能力，通过 GitLab 状态传达此信息. 这将指导撰稿人选择其他审稿人，从而帮助我们满足 SLO.

当然，如果您不在办公室并且已通过 GitLab.com 状态传达了此信息，则作者应意识到这一点，并自己找一位不同的审稿人.

当合并请求作者的阻止时间超过Review-response SLO 时，他们可以自由地通过 Slack 提醒审阅者或分配其他审阅者.

Customer critical merge requests

合并请求可能会因为被视为客户关键优先级而受益，因为这样做会使企业受益匪浅.

客户关键合并请求的属性：

  • 发展高级总监 （ @ clefelhocz1 ）是用于确定合并请求是否对客户至关重要的 DRI.
  • DRI 将customer-critical-merge-request标签分配给合并请求.
  • 要求与客户关键合并请求相关的审阅者和维护者一经做出此决定，便立即参与.
  • 需要优先处理涉及客户关键合并请求的人员的工作，以便他们有足够的时间专注于此.
  • 在处理客户关键的合并请求时，必须遵守 GitLab 的和流程，首先要特别注意家人和朋友，其次要注意，完成的定义，迭代以及准备就绪时发布.
  • 需要客户关键的合并请求，以免降低安全性，引入数据丢失风险，降低可用性或破坏每个流程的现有功能，从而优先考虑技术决策 .
  • 对于客户的关键请求，如果他们认为这将减少花费的合并时间（即使这可能会降低 ），则建议相关人员考虑除了异步（合并请求注释）之外，还同步进行协调（Zoom，Slack）.

如何进行代码审查可能会使新的参与者感到惊讶. 以下是一些代码审查示例，它们可以帮助您确定期望的方向.

“修改DiffNote以便将其重新用于设计” ：它包含了从换行符转折点到设计版本的推理，如果没有某个文件的先前版本，我们应该如何比较它们的所有内容（父级 vs.空白空白树） ）.

：MR 本身由 FE 和 BE 之间的协作组成，并记录了作者对审阅者的评论. 有一些缺点，一些有关信息的问题，最后还有一个安全漏洞.

“每个项目允许有多个存储库” ：ZJ 提到了这可能会影响的其他项目（工作马），并建议进行一些改进以保持一致性. James 的评论帮助我们提高了整体代码质量（使用委派&.这些类型的事情），并使代码更加健壮.

：MR 接触代码库多个部分的一个很好的示例. 尼克指出了一些有趣的例子，詹姆斯·洛佩兹（James Lopez）也加入了对进出口功能的关注.

很大程度上基于thinkbot 代码的审查指南 .