代码评审指南
本指南包含执行代码评审和让他人评审您代码的建议和最佳实践。
极狐GitLab 的所有合并请求,无论是由极狐GitLab 团队成员编写,还是由更广泛的社区成员编写,都必须通过代码评审过程以确保代码的有效性、可理解性、可维护性及安全性。
评审、批准和合并您的合并请求
在您开始之前:
- 您需要熟悉贡献接受标准。
- 如果您需要指导(例如,这是您的第一个合并请求),请随时询问合并请求教练。
一旦您有代码要评审,请评审者评审代码。 该评审者可以来自您的小组或团队,也可以是领域专家。 评审人可以:
- 为您提供所选解决方案和实施的不同意见。
- 帮助查找错误、逻辑问题或未发现的边缘情况。
如果合并请求很容易评审(例如,修复拼写错误或不改变行为或任何数据的微小重构),您可以跳过评审者步骤,直接让维护者评审。 否则,合并请求应首先由每个相关类别(例如后端或数据库)的评审者评审,因为维护者可能不具备相关的领域知识,并且也可以分散工作量。
根据您的合并请求涉及的区域,合并请求必须由一个或多个维护者批准。 已批准 按钮位于合并请求小部件中。
合并合并请求也需要维护者。如果需要由多个批准者批准,则由最后一个评审和批准的维护者进行合并。
阅读更多有关作者职责的内容。
评审者轮盘
Danger bot 会为您的合并请求所可能涉及的代码库的每个领域随机选择一个评审者和一个维护者。 它为开发人员评审者提供建议,如果您认为其他人更合适,您可以更改。面向用户的改动也需要进行 UX 评审,即使这个改动启用了功能标志。默认为推荐的 UX 评审者。
不针对安全群组下任何项目的默认分支的合并请求的审查请求不被计算在内。这些 MR 通常是反向移植的,维护者或评审者通常不需要太多时间来进行评审。
批准指南
如下面维护者职责部分所述,我们建议您让具有领域专业知识的维护者审批和合并您的合并请求。
验收清单
此清单鼓励合并请求 (MR) 的作者、评审者和维护者确认改动对质量、性能、可靠性、安全性、可观测性和可维护性的高影响风险进行了分析。
使用清单可以提高软件工程的质量。这个清单是一个简单的工具,可以支持和增强极狐GitLab 代码库贡献者的技能。
质量
- 您已根据代码评审指南对 MR 进行了自我评审。
- 对于此改动影响的代码,您认为自动化测试(测试指南)验证了对用户非常重要的功能。
- 如果现有的自动化测试没有涵盖上述功能,您已经添加了必要的附加测试或添加了一个议题来描述自动化测试的差距并将其链接到此 MR。
- 您已经考虑了此改动对 JihuLab.com 所托管的客户和私有化部署客户在技术方面的影响。
- 您已经考虑了此改动对系统的前端、后端和数据库部分的影响,并相应地应用了
~ux
、~frontend
、~backend
和~database
标签。 - 您已在所有支持的浏览器中测试过此 MR,或确定不需要此测试。 <!–1. You have confirmed that this change is backwards compatible across updates, or you have decided that this does not apply.
- You have properly separated EE content from FOSS, or this MR is FOSS only.
- 您已经考虑现有数据可能不同。例如,新模型验证可以打破现有记录。如果您尚未确认现有数据将通过验证,请考虑将对现有数据的验证设为可选而非必需。
- 如果测试通过但有警告,并且失败的作业包含文本
Flaky test '<path/to/test>' was found in the list of files changed by this MR.
,则您已修复此测试,或提供了证据解释为什么可以忽略这个不稳定的测试。
性能、可靠性和可用性
- 您确信此 MR 不会损害性能,或者您已请评审者帮助评估性能影响。 <!–1. You have added information for database reviewers in the MR description, or you have decided that it is unnecessary.
- 您已考虑本次改动的可用性和可靠性风险。
- 您已经考虑了基于未来预测增长的可扩展性风险。
- 您已考虑此改动对比普通客户拥有更多数据的大客户的性能、可靠性和可用性方面的影响。
可观察性检测
- 您已经做了完备的检测,通过可观察性来促进调试和主动性能改进。
文档
- 您已经包含了更新日志 trailer,或者您决定不需要包含。
- 您已经添加/更新了文档,或者决定不需要对此 MR 进行文档改动。
部署
- 您已经考虑为这个改动使用功能标志,因为这个改动风险较高。
- 如果您使用功能标志,您计划在生产环境中测试之前先在预部署环境中进行测试,并且您已考虑在将其推广到所有客户之前将其推广到一部分生产客户。 <!– - When to use a feature flag
- You have informed the Infrastructure department of a default setting or new setting change per definition of done, or decided that this is unnecessary.–>
合并请求作者的职责
合并请求作者的职责是找到最佳解决方案并实施。在整个代码评审生命周期中,作者或直接负责的个人始终作为指派人。如果您无法将自己设置为指派人,请让评审者帮助您设置。
在请求维护者评审以审核和合并之前,应该确信该合并请求:
- 实际上解决了要解决的问题。
- 是最合适的解决方式。
- 满足所有要求。
- 没有其他错误、逻辑问题、未发现的边缘情况,或已知漏洞。
如果想做好这一点,并避免与评审人进行不必要的意见交换,您需要遵循代码评审指南对自己的合并请求进行自我评审。 在自我评审中,尝试在线在 MR 中做出决定或权衡的地方添加评论,或者添加上下文解释以助于评审者理解。
为对自己的解决方案增强信心,作者应该在必要的时候让其他人参与调查和实施。
我们鼓励合并请求作者联系领域专家讨论不同的解决方案;联系产品经理和用户体验设计师评审实施,以澄清不清楚的地方或验证最终结果是否符合他们的想法;联系数据库专家获取有关数据模型或特定查询的意见;或者联系任何其他开发人员以获得对该解决方案的进一步评审。
如果您的合并请求涉及多个领域(例如,动态分析和 GraphQL),请向每个领域的专家寻求评审。
如果作者不确定合并请求是否需要领域专家的意见,这恰恰说明领域专家的意见必不可少。没有领域专家的意见,合并请求作者可能对自己的方案没有足够的信心。
在评审之前,作者被要求提交关于合并请求差异的评论,提醒评审者任何重要的事情以及任何需要进一步解释或注意的事情。可能需要评论的内容可以是:
- 添加了 linting 规则(RuboCop、JS 等)。
- 添加了一个库(Ruby gem、JS 库等)。
- 在不明显的地方,指向父类或方法的链接。
- 为补充改动而执行的任何基准测试。
- 潜在的不安全代码。
如果评审者需要任何项目、代码片段或其他资产来验证解决方案,请确保您在向他们请求评审之前,他们可以访问这些资产。
分配评审者时,以下做法可能有所帮助:
- 在 MR 中添加评论,指出您正在向评审人请求哪种类型的评审。
避免以下情况:
- 将 TODO 评论(上面提到的)直接添加到源代码中,除非评审者要求您这样做。
- 添加注释,仅解释代码的作用。如果添加了非 TODO 评论,应该解释为什么要添加这些代码,而不是代码是什么。
- 请求维护者对测试失败的合并请求进行评审。如果测试失败并且您必须请求评审,请确保您发表评论并提供解释。
- 通过电子邮件过多地提及维护者。如果您不能为合并请求添加评审者,可以在评论中使用
@
提及维护者,在所有其他情况下添加评审者就足够了。
这可以节省评审人的时间,并帮助作者及早发现错误。
评审者的职责
评审者负责评审所选解决方案的细节内容。
全面地评审合并请求。
一些合并请求可能需要领域专家来帮助处理细节。 评审者如果不是该领域的领域专家,可以执行以下任何操作:
- 评审合并请求并邀请一位领域专家进行额外评审。这位专家可以是评审者或维护者。
- 将 MR 转给他们认为更合适的另一位评审者。
- 如果没有可用的领域专家,则尽最大努力进行评审。
在以下情况中,您应该指导作者将合并请求进行拆分:
- 合并请求过大。
- 修复了不止一个问题。
- 实现不止一项功能。
- 具有导致额外风险的高复杂性。
作者可以选择请求当前的维护者和评审者评审拆分的 MR,或者请求一组新的维护者和评审者进行评审。
当您有信心 MR 满足所有要求时,您应该:
- 选择 批准。
-
@
提及作者以生成待办事项通知,并告知他们其合并请求已被评审和批准。 - 请求维护者进行评审。默认请求具有领域专业知识的维护者进行评审。
- 移除自己的评审者身份。
维护者的职责
维护者负责整个领域和产品范围内极狐GitLab 代码库的整体健康、质量和一致性。
因此,他们的评审主要集中在总体架构、代码组织、关注点分离、测试、DRYness、一致性和可读性等方面。
因为维护者的工作只取决于他们对整个极狐GitLab 代码库的了解,而不是任何特定领域的知识,所以他们可以评审、批准和合并任何团队和产品领域的合并请求。
维护者是确保合并请求的验收标准得到合理满足的直接负责个人(DRI)。 一般来说,质量是每个人的责任,但是 MR 的维护者有责任确保 MR 符合这些通用质量标准。
维护者在合并之前也会尽最大努力评审所选解决方案的细节,但他们不一定是领域专家,有时并不合适。 在这种情况下,他们会遵从作者和早期评审者的判断,专注于他们的主要职责。
如果一个开发人员碰巧也是维护者,且作为评审者参与了合并请求,建议不要将其选为维护者以进行最终批准和合并。
维护者应在合并前检查合并请求是否得到所需批准人的批准。如果仍在等待其他人的进一步批准,请将自己从评审者中删除,然后 @
提及作者并在评论中解释原因。如果需要您来合并代码,请继续担任评审者。
请注意,某些合并请求可能针对稳定分支。这些都是小概率事件。维护者不能合并这些类型的合并请求。
合并后,维护者应继续做为合并请求中列出的评审者。
内部使用评审者功能
2021 年 3 月 18 日,更新后的流程已完成,旨在高效且一致地对评审者功能进行内部使用。
以下是改动摘要,您在上面的内容中也有所了解。
- 合并请求作者和 DRI 仍然是指派人。
- 作者在需要时向评审者请求评审。
- 评审者在完成评审/批准后自行移除。
- 最后一个批准人在合并时保留为评审人。
最佳实践
所有人
- 礼貌友好。
- 明白且接受许多编程决策都是个人想法。讨论权衡、选择取舍并迅速达成一致。
- 温和提问而非强硬要求。 (”将这个命名为
:user_id
你觉得怎么样?”) - 请求解释。 (“我不太明白。能麻烦你解释一下吗?”)
- 避免选择性地拥有代码。 (“我的”、“不是我的”、“你的”)
- 避免使用可能被视为指代个人特质的术语。(“哑的”、“愚蠢的”)。假设每个人都是聪明且善意的。
- 清晰明确。人们并不总能在网上理解您的意思。
- 保持谦虚。 (“我不太确定、我们一起看看吧。”)
- 不要使用夸张的词语。 (“总是”、“从不”、“无尽的”、“没有任何事情”)
- 尽量不使用反讽。我们所做的一切都是公开的;熟人之间的善意玩笑在项目新人看来可能是刻薄且不友善的。
- 如果有太多“我不理解”或“替代解决方案”的评论,则建议进行一对一的沟通。沟通后将内容整理为 MR 的评论。
- 如果您向特定的人提问,请在评论开头
@
提及他们。这确保他们将通知级别为设置为“提及”时可以收到消息,且其他人明白他们不必回应。
评审您的合并请求
代码评审是一个可能需要多次迭代的过程,评审者可能不会第一次就提出所有意见。
- 您的代码的第一个评审者是您自己。在首次推送之前,请全文检查。MR 是否合理? 是否包含了与改动目的无关的内容?是否忘记删除调试代码?
-
编写详细描述。 某些评审者可能不熟悉代码库的产品特性或范围。详尽的描述有助于所有评审者理解您的请求并有效地进行测试。
- 如果您知道您的改动依赖于另一个合并,请在描述中注明并设置合并请求依赖项。
- 感谢评审人的建议。(”这个是个好建议,我更改一下。”)
- 评审对事不对人。我们评审的是代码,而不是您。
- 解释为什么会写某段代码。(”由于这些原因,所以我这样写的。如果我重命名这个类/文件/方法/变量是不是更清楚些?”)
- 将不相关的改动和重构提取到以后的合并请求/议题中。
- 尝试理解评审者的观点。
- 尽量回复每条评论。
- 合并请求作者仅解决他们已经按照评论要求更改完毕的主题。如果有未解决的回复、未解决的主题、建议和其他问题,主题应该留给评审者解决。
- 不应假定所有反馈都要求在合并之前将其建议的改动合并到 MR 中。MR 作者和评审者会自行判断是否需要立即并入到本次的 MR 中,或者是否应该合并当前 MR,创建后续议题以解决反馈问题。
- 将基于前几轮反馈的提交作为独立的提交推送到分支。在分支准备好合并之前不要压缩。评审者应该能够根据之前的反馈读取个别更新。
- 准备好进行另一轮评审后,请评审者重新评审。如果您无法请求评审,请
@
提及评审者。
请求评审
当您准备好评审您的合并请求时,您应该请求初步评审。
当合并请求有多个评审领域时,建议您指定评审者评审哪个领域,以及在哪个阶段(第一或第二)。
这将帮助有资格作为多个领域的评审者的团队成员了解他们被要求评审哪个领域。
例如,当合并请求同时涉及 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
标签。这意味着您的合并请求已准备好接受评审,任何评审者都可以进行评审。建议仅在时间不紧急时才使用该标签,并确保将合并请求分配给评审者。
当您的合并请求第一次被评审者批准时,该 MR 会传给维护者。您应该默认选择具有领域专家经验的维护者,否则请遵循评审者轮盘推荐或使用 ready for merge
标签。
有时,维护者可能无法评审。他们可能不在办公室或没有时间。 您可以而且应该在他们的个人资料中查看维护者是否可以评审。如果评审者轮盘推荐的维护者无法评审,请从该列表中选择其他人。
评审合并请求是作者的责任。如果 MR 在 ready for review
停留的时间过长,建议请特定评审人员进行评审。
评审合并请求
了解为什么需要进行改动(修复错误、改进用户体验、重构现有代码)。然后:
- 尽量评审全面以减少迭代次数。
- 对您有强烈想法和没有强烈想法的内容进行沟通。
- 寻找既能解决问题又能简化代码的方法。
- 提供替代实现方法,但假设作者已经考虑过这些方法。(“您如何看待在这里使用自定义验证器?”)
- 尝试理解作者的观点。
- 检出分支,并在本地测试改动。您可以决定要执行多少手动测试。 您的测试可能会引入自动化测试。
- 如果某段代码您不理解,请如实说明。很可能其他人也不理解。
- 确保作者清楚他们如何解决建议。
- 考虑使用常规评论格式表达您的意思。
- 对于非强制性建议,使用”(非阻塞)”,这样作者就知道他们可以选择性地在合并请求中解决或在稍后阶段跟进。
- 确保没有开放的依赖项。检查链接议题是否存在阻碍。必要时可与作者澄清。 如果被一个或多个打开的 MR 阻塞,请设置一个 MR 依赖项。
- 留下一轮评论意见后,将评论意见汇总对 MR 作者来说很有帮助,例如“我没有其他问题了”或者“只是有几件事要解决”。
- 让作者知道在您评审后是否需要进行改动。
合并合并请求
在做出合并决定之前:
- 设定里程碑。
- 确认应用了正确的 MR 类型标签。
- 考虑来自 danger bot、代码质量和其他报告的警告和错误。 除非可以为违规行为提出强有力的理由,否则在合并前应该解决这些问题。 如果 MR 合并时带有失败的作业,则必须留下评论说明原因。
- 如果 MR 同时包含质量和非质量相关的改动,则在测试软件工程师批准质量相关的改动后,MR 应由相关维护者合并以进行面向用户的更改(后端、前端或数据库)。
MR 至少需要一个维护者的批准才能合并。MR 作者和 向 MR 添加提交的人员无权批准或合并 MR,必须找到一个没有对 MR 做出过贡献的维护者来批准和合并。
为了在 gitlab-cn/jihulab
中实施此政策,我们启用了以下设置,以确保 MR 获得顶级 CODEOWNERS 维护者批准:
在某些情况下,例如本地变基或应用建议,这些建议被认为与添加提交相同,并且可以重置现有批准。从 UI 变基或使用 /rebase
快速操作时,不会删除批准。
准备合并时:
- 当合并请求有很多提交时,考虑使用压缩和合并功能。 合并代码时,维护者应该只在作者已经设置了这个选项时使用压缩功能,或者如果合并请求的提交历史较为混乱,压缩提交会更有效。如果 MR 只有几个提交,则可以尊重作者的设置,不进行压缩。
- 使用合并请求的”流水线”选项卡中的
Run pipeline
按钮启动新的合并请求流水线,并启用”管道成功时合并”(MWPS)。 请注意:- 如果最新的流水线是在合并请求被批准之前创建的,启动一个新的流水线以确保完整的 RSpec 套件已经运行。仅当合并请求不包含任何后端改动时,您才可以跳过此步骤。
- 如果最新的合并结果流水线是在不到 6 小时前创建的,并且是在不到 2 小时前完成的,您可能会在不启动新流水线的情况下合并,因为合并请求足够接近
main
。
- 当您将 MR 设置为”流水线成功时合并”,您应该负责之后发现的任何内容的后续改动。
- 对于已设置压缩和合并的合并请求,压缩提交的默认提交消息取自合并请求标题。 我们鼓励您在合并之前选择一个包含更多提交信息的提交。
多亏了合并结果流水线,作者不再需要像以前那样频繁地重新变基分支(仅当存在冲突时),因为合并结果流水线已经包含来自 main
的最新改动。
这会导致更快的评审/合并周期,因为维护者不必要求最终的变基:相反,他们只需要启动 MR 流水线并设置 MWPS。
通过在创建流水线时针对最新的 main
测试合并结果,此步骤使我们非常接近实际的合并列车功能。
社区贡献
在评审更广泛的社区贡献者添加的合并请求时:
- 特别注意新的依赖项和依赖项更新,例如 Ruby gems 和 Node 包。
虽然对
Gemfile.lock
或yarn.lock
等文件的改动可能看起来微不足道,但这些改动可能会获取恶意包。 - 评审链接和图像,尤其是在文档 MR 中。
- 如有疑问,请来自
@gitlab-com/gl-security/appsec
的人在手动启动合并请求流水线之前评审合并请求。 - 仅当合并请求可能包含在当前里程碑中时才设置里程碑。这是为了避免混淆何时合并,并避免在尚未准备好时过于频繁地移动里程碑。
如果 MR 源分支落后于目标分支 1,000 多次提交:
- 如果 MR 启用了”允许来自可以合并到目标分支的成员的提交”,请作者对其进行变基,或者自己对其进行变基。
- 在最近改动的上下文中评审 MR 有助于防止隐藏的运行时冲突并促进一致性。根据改动的性质,如果 MR 少于 1,000 次提交,您可能还需要变基。
- 强制推送可能会让贡献者不清楚状况,因此您最好跟其他人沟通您进行了变基,或者在贡献者积极处理 MR 时先与他们核实。
- 变基通常可以在极狐GitLab 中使用
/rebase
快速操作来完成。
接管社区合并请求
当 MR 需要进一步改动但作者长时间没有回应,或者无法完成 MR 时,极狐GitLab 可以接管。极狐GitLab 工程师(通常是合并请求教练)将:
- 在他们的 MR 中添加一条评论,说明您将进行接管以便能够将其合并。
- 在他们的 MR 中添加标签
~"coach will finish"
。 - 从主分支创建一个新的功能分支。
- 将他们的分支合并到您的新功能分支中。
- 创建一个新的合并请求,将您的功能分支合并到主分支中。
- 社区 MR 和您的 MR 链接起来,并打
~"Community contribution"
标签。 - 进行任何必要的最终调整并通知贡献者,让他们能够评审您的改动,并让他们知道他们的内容正在合并到主分支中。
- 确保内容符合所有合并请求指南。
- 遵循我们对任何合并请求所做的常规评审流程。
正确的平衡
代码评审时最困难的一件事是评审者需要在代码评审过程中找到一个干预的平衡点。
- 学习如何找到正确的平衡点需要时间;这就是评审者花费一些时间评审合并请求后成为维护者的原因。
- 发现错误很重要,但思考良好的设计同样也很重要。构建抽象和良好的设计可以隐藏复杂性并使未来的更改更容易。
- 要求作者更改设计有时意味着完全重写贡献的代码。最好在这样做之前询问其他维护者或评审者,但是当您认为必须要这样做时要鼓起勇气去做。
- 正确地做事和现在做事是有区别的。理想情况下,我们应该正确地做事,但在现实世界中我们同样也需要现在做事。例如我们应该尽快修复安全问题。应避免要求作者在合并请求中进行紧急修复的主要重构。
- 一件事情今天做到八分好也比明天做到十分好更重要,但今天做到三分好远不及明天做到六分好更重要。这中间的取舍和平衡需要您自行掌握。当您无法找到合适的平衡点时,请询问其他人的意见。
极狐GitLab 特定的关注点
极狐GitLab 被用在很多地方。许多用户使用我们的 Omnibus packages,但有些使用 Docker 镜像,还有其他可用的安装方法。 JihuLab.com 本身就是一个很大的企业版实例。以下是一些含义:
- 应该对查询改动进行测试以确保其不会在 JihuLab.com 上带来性能危害:
- 在本地生成大量数据会有所帮助。
- 询问 JihuLab.com 的查询计划是最可靠的验证方式。
-
数据库迁移必须是:
- 可逆的。
- 性能与 JihuLab.com 相当 - 如果您不是很确定,那么请维护人员在预部署环境中测试迁移的性能影响。
<!–1. Categorized correctly:
- Regular migrations run before the new code is running on the instance.
- Post-deployment migrations run after the new code is deployed, when the instance is configured to do that.
- Batched background migrations run in Sidekiq, and should be used for migrations that exceed the post-deployment migration time limit GitLab.com scale.–> <!–1. Sidekiq workers cannot change in a backwards-incompatible way:
- Sidekiq queues are not drained before a deploy happens, so there are workers in the queue from the previous version of GitLab.
- If you need to change a method signature, try to do so across two releases, and accept both the old and new arguments in the first of those.
- Similarly, if you need to remove a worker, stop it from being scheduled in one release, then remove it in the next. This allows existing jobs to execute.
- Don’t forget, not every instance is upgraded 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.–>
- 缓存值可能会跨版本持续存在。如果要更改缓存值返回的类型(例如,从字符串或 nil 到数组),请同时更改缓存键。 <!–1. Settings should be added as a last resort. See Adding a new setting to GitLab Rails.
- File system access is not possible in a cloud-native architecture. Ensure that we support object storage for any file storage we need to perform. For more information, see the uploads documentation.–>
参考内容
本文部分内容参考 thoughtbot
代码评审指南。