Options for transition from email based code review to GitHub PR code review

[mdkinney]

The following material was discussed in the EDK II Community Meeting on build, tools, CI on June 13, 2022

EDK II Source Code Change Attributes

• Historically EDK II source code changes split into a patch series
  • Single patch is not allowed to modify multiple repositories (e.g. edk2 + edk2-platforms)
  • Single patch is not allowed to modify multiple packages
  • Break complex changes into smaller chunks to reduce review burden
  • Break up due to email patch size limits
  • Split interface changes from implementation changes
  • Split functional changes from cosmetic (formatting) changes
    • Adding Uncrustify has reduced cosmetic change patches, but does not eliminate them
• Challenges
  • GitHub Flow not optimized for patch series
    • https://docs.github.com/en/get-started/quickstart/github-flow
  • CODEOWNERS assigns reviewers based on file changes in PR (not commit scoped)
    • https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners

EDK II Code Change Options

Do not split complex changes into multiple commits

• Use GitHub Flow “as is” with CODEOWNERS and Squash/Merge.
• Good for small changes that do not cross repo/package boundaries
• Challenges: Not clear which portions need to be reviewed by which reviewers
• Explore CODEOWNERS team assignments and PR setting to wait for CODEOWNERS to address cross package reviews

Break complex changes into multiple PRs

• Single PR per change.
• Use GitHub Flow “as is” with CODEOWNERS and Squash/Merge.
• Challenges to group related PRs and order them correctly
• Significantly increases number of PRs to track

Break complex changes into Stacked PRs (Dependent PRs)

• Single commit per branch with branches from branches
  • https://blog.logrocket.com/using-stacked-pull-requests-in-github/
  • https://github.com/marketplace/stacked-pull-requests
  • https://github.com/marketplace/dpulls
• Increases submitter burden to manage feedback
• Rebase all branches from point of change due to feedback
• Significantly increases number of PRs to track
• Likely ok for 2-5 commits, but cumbersome for more complex changes

One PR per Patch Series – Open new PR for any code changes in series

• Rebase ff merge.
• Perform review at commit scope instead of PR scope
• Webhook required to assign reviewers/maintainers to each commit
• Webhook required to track review approvals through comments
• Webhook email archive required because commit scoped review comments may be lost
• Increases number of abandoned PRs

One PR per Patch Series – Forced push for any code change in series

• Rebase ff merge.
• Perform review at commit scope instead of PR scope
• Webhook required to assign reviewers/maintainers to each commit
• Webhook required to track review approvals through comments
• Webhook email archive required because commit scoped review comments may be lost
• Reduced total number of PRs

One PR per Patch Series – Forced push for any code change in series

• Rebase ff merge.
• Use CODEOWNERS to assign reviewers/maintainers to PR based on file changes
• Perform review at commit scope or PR scope as appropriate
• All comments must be resolved to merge
• Reviewers/Maintainer responsible for finding commits they are responsible for
  • Approved response means approval of entire PR and commits they are responsible for
  • OPEN: How to map Acked-by usage from a maintainer to GitHub PR Approval
• Webhook email archive required because commit scoped review comments may be lost
• Reduces total number of PRs
• OPEN: Are Rb, Ab, Tb tags in git commit history still required if that information is available through GitHub PR history.
• Explore CODEOWNERS team assignments and PR setting to wait for CODEOWNERS to address cross package reviews