Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Michael D Kinney


I agree that the first version should rerun CI checks
on every time commits are added to a PR or there is a
forced push to the PR.

Perhaps we should use Draft Pull Requests as a way
to indicate the content is not ready for code review
or CI checks yet.

We also want emails added to the email archive when
the pull request is either abandoned or merged.
merify can add comments to a PR that are picked up
by the webhook.

I agree with reducing the number of services required.
There was feedback from Laszlo related to rebase for
pull requests using the current CI process. I will
do more investigations of GitHub features, webhook
features, and Mergify features to see if there is
simpler overall solution.


-----Original Message-----
From: Rebecca Cran <rebecca@...>
Sent: Sunday, May 10, 2020 2:44 PM
To:; Kinney, Michael D
Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull
Request based Code Review Process


On 5/10/20 3:29 PM, Michael D Kinney wrote:

There is no difference between CI checks run during
code review
and the final CI checks before merge. I think it is
an interesting
conversation to decide how many times those CI checks
should be
run and if they should run automatically on every
change during
review or on demand.
I'd suggest following what other Github projects do,
which I think is to
run the CI checks automatically on every change that's
made in a pull
request - I don't know if it might also be necessary to
run them during
the merge, if master has changed in the meantime. That
gives the
_submitter_ feedback about any changes they need to
make, instead of
having to wait until the maintainer tells them their
change has broken
something: it speeds up the development process.

Mergify is more flexible. We want to make sure the
git history
is linear with not git merges and supports both
single patches
and patch series without squashing. GitHub merge
button by
default squashes all commits into a single commit.
Wouldn't disabling all but "Allow rebase merging" do
the same thing
without the additional potential failure point? Though
it sounds like
we've resolved the problems with Mergify, so it's not

Rebecca Cran

Join to automatically receive all group messages.