Re: [edk2-devel] GitHub PR Code Review process now active

["Guo Dong" <guo.dong@intel.com>]

Hi Mike,

Glad to see EDK2 PR code review process is active.
In Slim Bootloader project, it runs BaseTools/Scripts/PatchCheck.py to check the PR commit message when running QEMU CI test  
Maybe you could refer https://github.com/slimbootloader/slimbootloader/blob/48a24b87824321c053cccf367c7c3637ff581fdf/.azurepipelines/azure-pipelines.yml#L38 to check if EDK2 could use similar check.

Thanks,
Guo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Wednesday, June 5, 2024 3:21 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] GitHub PR Code Review process now active

Hello,

The PR code review process has been active for a little over a week now.

There have been about 17 PRs merged since the switch and it appears to have been mostly working well. I also note that the emails per day on this mailing list is much smaller as the code reviews have migrated to PRs.

A few issues have been noted:

1) Contributors that are not EDK II Maintainers do not have permissions
   to assign reviewers 

   * Mitigation #1: EDK II Maintainers review new PRs and assign reviewers
   * Mitigation #2: Use CODEOWNERS to auto assign maintainers.  WIP.

* EDK II Maintainers must review the commit message in each commit and
  to make sure it follows the required commit message format and has
  an appropriate Signed-off-by tag. GitHub does not provide a way to
  provide review comments for a commit message. Instead, feedback on
  commit messages must be provided in the main PR conversation and quote
  commit message that requires changes as required.

* Slow CI Performance

  This appears to be due to longer than expected queues in Azure Pipelines.
  Azure Pipelines is working through the backlog. It may help if the
  number of requests to rebase and number of new commits to open PRs are
  reduced. The Tools/CI team will continue to collect data and determine
  if other changes are needed to reduce the CI overhead.

* Some PRs have been merged using the "Rebase and Merge" button in the
  PR after all required reviews completed and all CI checks pass. Instead,
  the "push" label should continue to be used. There does not appear to be
  any unexpected side effects from the "Rebase and Merge" button, but that
  option is not available if the PR needs to be rebased. This is what
  Mergify handles through a merge queue, so the easiest way to merge right
  now is the "push" label.

  If the most recent commit was not performed by an EDK II Maintainers, then
  Mergify attempt to rebase may fail.

    Mitigation #1: EDK II Maintainer perform a rebase
    Mitigation #2: Update Mergify to use a bot account with write permission
                   to perform rebase operations.

  There was feedback earlier in the year that the git commit history does
  not indicate which maintainer was the committer.  Instead it always shows
  Mergify.

  The use of GitHub Merge Queues will be evaluated to see if it can be used
  instead of Mergify and remove the need for the "push" label and allow the
  "Rebase and Merge" button to be used and avoid the Mergify permission issues.

* Some PRs do not complete all CI checks waiting for "Workflow Approval".
  this can occur when a PR is updated by an outside collaborator that does
  not have any previous "Workflow Approvals" accepted.

  Mitigation #1: EDK II Maintainers review PRs and accept the "Workflow Approval"
                 if the PR looks like a good change request.
  Mitigation #2: Relax the edk2 repo configuration settings related to workflows

* When a PR needs to be rebased, there are 2 options available through the
  Web UI:

  * Update with merge commit (Never use - generates PatchCheck errors)
  * Update with rebase (Only use this one)

  If Update with merge commit is accidently applied, then redo again
  Using "Update with rebase"

Please provide feedback if you are seeing other issues or have other suggestions to improve the process.

Thanks,

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, June 3, 2024 12:38 PM
> To: Neal Gompa <ngompa13@gmail.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> 
> The reason to allow a draft PR is to allow contributors to run all the 
> CI tests to see if they pass and resolve issues before starting a review.
> 
> The CI tests include combinations of OS/compiler that not all 
> contributors have available.
> 
> Mike
> 
> > -----Original Message-----
> > From: Neal Gompa <ngompa13@gmail.com>
> > Sent: Monday, June 3, 2024 11:47 AM
> > To: devel@edk2.groups.io; Kinney, Michael D 
> > <michael.d.kinney@intel.com>
> > Subject: Re: [edk2-devel] GitHub PR Code Review process now active
> >
> > Hmm, I don't see a setting for it anymore, maybe that's not a thing
> anymore?
> >
> > I seemingly recall that draft PRs didn't get CI runs, but if that's 
> > not a thing anymore, then that's fine.
> >
> > That said, draft PRs cannot be reviewed, so we should not be telling 
> > people to make draft PRs.
> >
> >
> > On Mon, Jun 3, 2024 at 12:26 PM Michael D Kinney via groups.io
> >
> > <michael.d.kinney=intel.com@groups.io> wrote:
> > >
> > > CI jobs are dispatched to both GitHub Actions and Azure Pipelines.
> > >
> > > For Draft PRs, I see both GitHub Actions and Azure Pipelines jobs
> running.
> > >
> > > This must imply that edk2 repo allows this.  Do you happen to know 
> > > where this is configurable or a link to GitHub docs for configuration?
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Neal Gompa <ngompa13@gmail.com>
> > > > Sent: Monday, June 3, 2024 9:13 AM
> > > > To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> > > > Subject: Re: [edk2-devel] GitHub PR Code Review process now 
> > > > active
> > > >
> > > > On Tue, May 28, 2024 at 2:53 PM Michael D Kinney via groups.io 
> > > > <michael.d.kinney=intel.com@groups.io> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > The GitHub PR code review process is now active.  Please use 
> > > > > the new PR based code review process for all new submissions 
> > > > > starting today.
> > > > >
> > > > > * The Wiki has been updated with the process changes.
> > > > >
> > > > >   
> > > > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> > Development-
> > > > Process
> > > > >
> > > > >   Big thanks to Michael Kubacki for writing up all the
> > > > >   changes based on the RFC proposal and community discussions.
> > > > >
> > > > >   We will learn by using, so if you see anything missing or
> > > > >   incorrect or clarifications needed, please send feedback
> > > > >   here so the Wiki pages can be updated quickly for everyone.
> > > > >
> > > > > * The edk2 repo settings have been updated to require
> > > > >   a GitHub PR code review approval before merging and
> > > > >   all conversations must be resolved before merging.
> > > > >
> > > > > * A PR has been opened that removes the requirement for
> > > > >   Cc: tags in the commit messages and is the first PR
> > > > >   that will use the new process. This PR needs to be
> > > > >   reviewed and merged to support the revised commit
> > > > >   message format.
> > > > >
> > > > >   https://github.com/tianocore/edk2/pull/5688
> > > > >
> > > > >   
> > > > > https://github.com/tianocore/tianocore.github.io/wiki/Commit-
> Message-
> > > > Format
> > > > >
> > > > > * Please use "Draft" PRs to run CI without any reviews.
> > > > >   Once ready for reviews, convert from "Draft" to
> > > > >   "Ready for Review".
> > > > >
> > > >
> > > > Generally GitHub doesn't allow CI to run on PRs created as draft 
> > > > pull requests. Was this changed for edk2?
> > > >
> > > >
> > > > --
> > > > 真実はいつも一つ！/ Always, there's only one truth!
> > >
> > >
> > > 
> > >
> > >
> >
> >
> > --
> > 真実はいつも一つ！/ Always, there's only one truth!







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119492): https://edk2.groups.io/g/devel/message/119492
Mute This Topic: https://groups.io/mt/106355103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-