Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

["Sean" <sean.brogan@microsoft.com>]

Laszlo/Mike,

The idea that the maintainer must create the PR is fighting the optimized github PR flow.  Github and PRs process is optimized for letting everyone contribute from "their" fork while still protecting and putting process in place for the "upstream".  

Why not use github to assign maintainers to each package or filepath and then let contributors submit their own PR to edk2 after the mailing list has approved. Then the PR has a policy that someone that is the maintainer of all files changed must approve before the PR can be completed  (or you could even set it so that maintainer must complete PR).   This would have the benefit of less monotonous work for the maintainers and on rejected PRs the contributor could easily update their branch and resubmit their PR.  

Thanks
Sean

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Laszlo Ersek via Groups.Io
Sent: Tuesday, September 3, 2019 9:55 AM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; rfc@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

On 09/03/19 18:41, Ni, Ray wrote:
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo 
>> Ersek
>> Sent: Tuesday, September 3, 2019 6:20 AM
>> To: Ni, Ray <ray.ni@intel.com>; rfc@edk2.groups.io; 
>> devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Kinney, 
>> Michael D <michael.d.kinney@intel.com>
>> Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous 
>> Integration Phase 1
>>
>> On 09/03/19 05:39, Ni, Ray wrote:
>>> Can someone draw a flow chart of who does what to help clarify?
>>
>> I don't see this as a huge change, relative to the current process.
>>
>> Before, it's always been up to the subsys maintainer to apply & 
>> rebase the patches locally, pick up the feedback tags, and run at 
>> least *some* build tests before pushing.
>>
>> After, the final git push is not to "origin" but to a personal branch 
>> on github.com, and then a github PR is needed. If that fails, notify 
>> the submitter. That's all, as far as I can see.
>>
>>> It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers).
>> Important it is I agree but boring it is as well I have to say.
>>
>> Oh, it *is* absolutely boring.
>>
>>> Are maintainers still needed to be picked up/chosen/promoted from developers?
>>
>> Would you trust a person to apply, build-test, and push your 
>> UefiCpuPkg patches, if that person had *zero* UefiCpuPkg development experience?
>> (Or even zero edk2 development experience?)
> 
> Can we change the process a bit?
> 1. maintainers created pull requests on behave of the patch owners 2. 
> patch owners can be notified automatically if pull requests fail

I believe this can work if:
- the submitter has a github account
- the maintainer knows the submitter's account name
- the maintainer manually subscribes the submitter to the PR
  (I have never tried this myself)

> 3. patch owners update the pull requests
>     (I am not familiar to pull requests. I assume the rights of 
> modifying pull requests and creating pull requests are separated. Are 
> they?)

PRs should not be updated. If there is a CI failure, the PR should be rejected. If that happens automatically, that's great. If not, then someone must close the PR manually. If the patch series submitter can do that, that's great (I have no idea if that works!) If only the PR owner (= maintainer) can close the PR, then they should close it.

Either way, the patch author will have to submit a v2 to the list. When that is sufficiently reviewed, the same process starts (new PR opened by maintainer).

> So, maintainers only need to initiate the pull requests.

I hope this can actually work. We need to test this out first.

> It assumes when pull requests are initiated, everyone at least agrees the justifications of the changes are valid and the ways the changes are done.

I agree fully about this. If the CI check completes, then the changes are pushed to master automatically! (Only if the push is a fast-forward
-- otherwise the PR is rejected again. GitHub will not be allowed to merge or rebase withoout supervision.)

Thanks
Laszlo