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.d...@intel.com> > Sent: Wednesday, June 5, 2024 6:22 PM > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@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.kin...@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.kin...@intel.com> > > Sent: Monday, June 3, 2024 12:38 PM > > To: Neal Gompa <ngomp...@gmail.com>; devel@edk2.groups.io > > Cc: Kinney, Michael D <michael.d.kin...@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 <ngomp...@gmail.com> > > > Sent: Monday, June 3, 2024 11:47 AM > > > To: devel@edk2.groups.io; Kinney, Michael D > > > <michael.d.kin...@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....@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 <ngomp...@gmail.com> > > > > > Sent: Monday, June 3, 2024 9:13 AM > > > > > To: devel@edk2.groups.io; Kinney, Michael D > > <michael.d.kin...@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....@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
