Re: [edk2-devel] EDK II Maintainers - EDK II CI is now active on edk2/master

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

Hi Laszlo,

Comments below.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, November 28, 2019 4:01 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; Sean Brogan
> <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
> CI is now active on edk2/master
> 
> On 11/27/19 20:03, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > Do you want to see the presence of the 'push' label
> in a notification
> > when the PR is created, or just when the checks have
> been completed in
> > either the pass or the fail state?
> 
> I'd like to see the purpose of the PR in the first
> notification email that is sent out about the PR.
> Whoever submits the PR clearly knows what they want
> with the PR, so they should communicate it. Placing
> some kind of token in the subject of the email(s) would
> be great (very visible).
> 
> > A PR may be created initially without the 'push'
> label.
> 
> Yes, I noticed that when I first experimented with the
> feature. I found it a bit surprising (but not a
> problem).
> 
> > The EDK II Maintainer can resolve any issues in that
> PR using forced
> > pushes to restart the checks with the issues
> resolved.
> 
> Yes, this is valid for a personal CI build. (Keep
> trying until it passes.)
> 
> > When all check pass, the maintainer can then set the
> 'push' label and
> > re-open the PR.  Then the checks will run again and
> the PR will be
> > merged.
> 
> This -- that is, trying to push until it succeeds -- is
> an invalid pattern for merging patches into edk2
> master, I believe.

I disagree.  As long as email code reviews are done on
the modified patch set and approved before the 'push'
label is set, this is a valid use of the CI system.

> 
> I seem to recall an agreement on the list that, in case
> of conflicts, the revised patch set -- with the
> conflicts resolved -- should be reposted to the list
> for review. Or else, minimally, the maintainer should
> report back with an explanation, or even the actual
> diff, of the
> (minimal!) changes the maintainer had to incorporate,
> to resolve the conflicts.

I agree.  Must always go through code review if changes
are made without reviewers consent.

> 
> I believe we discussed this in connection to whether
> we'd allow github.com to auto-resolve conflicts. We
> said we wouldn't; every conflict would need human
> inspection. And if changes were necessary, relative to
> the reviewed patch series, those should
> 
> - either be minimal; implemented, and also reported
> back to the list, by the maintainer,
> - or more extensive, and reposted by the original
> contributor.

Agree.

> 
> I'd like as few as possible code changes to happen
> outside of the normal review process. (Note: I'm not
> saying "outside of the mailing list" -- once github.com
> satisfies all the requirements, our normal review
> process may become github.com based. And then, on that
> platform, significant conflict resolution should also
> not occur without re-reviewing.)

I am not suggesting any changes to the current code review 
requirements.  A developer/maintainer can initially submit
a PR without the 'push' label.  If any checks fail, and
code changes are required, another round of email based 
reviews are required (v2, v3, etc).  Once those reviews
are approved, the updated patch series can be forced pushed
to the developer/maintainer branch to verify the changes
pass all checks or not and repeat as needed.  All of this
on a single PR.

One example of a failure that does not require code review
is a conflict because another PR was processed just 
before the current PR was submitted.  The maintainer needs
to sync their branch with master and resubmit the PR.  If no
code or commit message changes are required, then no additional
email review is required. This is the same as step 5 in the
following section that also does not require another round
of reviews:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project

> 
> > In this use case what email notifications would you
> like to see?
> 
> So this question doesn't seem to apply. It's OK if the
> maintainer does not add the "push" label immediately
> (given the UI, it's not even possible to do so). But a
> PR should be forever associated with a single purpose
> at "birth" (personal CI build, or actual push attempt),
> and the notification emails should reflect that
> purpose, from the start.
> 
> (IOW, I don't think it makes sense to "upgrade" a PR
> from "personal CI build" to "push to master".)

I disagree.  I think this is a good use case and helps 
minimize the total number of PRs in the PR history.  I am
not suggesting that this is the required use case.  I am
ok with a single PR handling multiple rounds of code 
reviews or multiple PRs for multiple rounds of code reviews.
However, if there are multiple PRs, we need to know that
the PRs are related to each other.

So my question still stands.  What notifications would
you like to see if we have the use case of a single PR
with multiple rounds of reviews and a transition from 
a PR without the 'push' label to a PR with a 'push' label?

> 
> Thanks!
> Laszlo