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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Guo,

Thanks.  Edk2 is running PatchCheck.py as part of CI.

The Maintainer still needs to verify the commit message contents
even if it passes PatchCheck.py.

Mike

> -----Original Message-----
> From: Dong, Guo <guo.dong@intel.com>
> Sent: Wednesday, June 5, 2024 6:22 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] GitHub PR Code Review process now active
> 
> 
> 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/48a24b87824321c053cccf3
> 67c7c3637ff581fdf/.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 (#119493): https://edk2.groups.io/g/devel/message/119493
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]
-=-=-=-=-=-=-=-=-=-=-=-