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 (#119488): https://edk2.groups.io/g/devel/message/119488
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]
-=-=-=-=-=-=-=-=-=-=-=-
