Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1
Sean, There may be many ways to improve the process and reduce the work maintainers perform. So these are ideas we can explore further going forward. I will add the concept of a non-maintainer opening the PR to the Wiki. In order to get the improvements to code quality from CI enabled as quickly as possible, I recommend we limit PRs to maintainers. Thanks, Mike > -Original Message- > From: Laszlo Ersek > Sent: Tuesday, September 3, 2019 10:46 AM > To: Sean Brogan ; > r...@edk2.groups.io; Ni, Ray ; > devel@edk2.groups.io; Gao, Liming > ; Kinney, Michael D > > Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II > Continuous Integration Phase 1 > > On 09/03/19 19:09, Sean Brogan wrote: > > 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. > > I'll let Mike respond. > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47677): https://edk2.groups.io/g/devel/message/47677 Mute This Topic: https://groups.io/mt/33122986/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1
On 2019-09-03 10:41, Ni, Ray wrote: > 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 > 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?) > > So, maintainers only need to initiate the pull requests. 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. In other projects I've worked on, patch owners initiate the pre-commit build/test, and either comment on the review (ideally with link to the results) about its status, or have the CI system comment automatically on the review page/thread. Once the maintainer has signed off on the patch, it can then be sent for gatekeeping, which is either a manual process whereby the repo owner (or subsystem maintainer) checks the review, runs any additional tests they want etc. - or via a 'land' command (for example with Review Board it's "rbt land") which automates the checks and pushes the changeset. I understand that with Azure Pipelines and how it integrates with Github, along with the fact that this project doesn't use pull requests or any other review automation, means things are probably going to be pretty different to most other projects for now. -- Rebecca Cran -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46836): https://edk2.groups.io/g/devel/message/46836 Mute This Topic: https://groups.io/mt/33122986/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1
On 09/03/19 19:09, Sean Brogan wrote: > 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. I'll let Mike respond. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46722): https://edk2.groups.io/g/devel/message/46722 Mute This Topic: https://groups.io/mt/33122986/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1
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: r...@edk2.groups.io On Behalf Of Laszlo Ersek via Groups.Io Sent: Tuesday, September 3, 2019 9:55 AM To: Ni, Ray ; devel@edk2.groups.io; r...@edk2.groups.io; Gao, Liming ; Kinney, Michael D 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 On Behalf Of Laszlo >> Ersek >> Sent: Tuesday, September 3, 2019 6:20 AM >> To: Ni, Ray ; r...@edk2.groups.io; >> devel@edk2.groups.io; Gao, Liming ; Kinney, >> Michael D >> 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46715): https://edk2.groups.io/g/devel/message/46715 Mute This Topic: https://groups.io/mt/33122986/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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 On Behalf Of Laszlo Ersek >> Sent: Tuesday, September 3, 2019 6:20 AM >> To: Ni, Ray ; r...@edk2.groups.io; devel@edk2.groups.io; >> Gao, Liming ; Kinney, >> Michael D >> 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46713): https://edk2.groups.io/g/devel/message/46713 Mute This Topic: https://groups.io/mt/33122986/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1
> -Original Message- > From: devel@edk2.groups.io On Behalf Of Laszlo Ersek > Sent: Tuesday, September 3, 2019 6:20 AM > To: Ni, Ray ; r...@edk2.groups.io; devel@edk2.groups.io; > Gao, Liming ; Kinney, > Michael D > 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 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?) So, maintainers only need to initiate the pull requests. 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. Thanks, Ray -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46711): https://edk2.groups.io/g/devel/message/46711 Mute This Topic: https://groups.io/mt/33122986/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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?) Maintainers don't have to be intimately familiar with the subsystem that they are pushing a given set of patches for, but every bit of knowledge helps. So the answer is "technically no, but you'll regret it". Maintainership is *always* a chore. It always takes away resources from development activities, for the maintainer. In most projects, people alternate in a given maintainer role -- they keep replacing each other. The variance is mostly in how frequent this alternation is. In some projects, the role belongs to a group, and people cover the maintainer responsibilities in very quick succession. In other projects, people replace each other in maintainer roles when the current maintainer gets tired of the work, and steps down. Then someone volunteers to assume the role. Either way, the new subsys maintainer is almost always a seasoned developer in the subsystem. Thanks, Laszlo > >> -Original Message- >> From: r...@edk2.groups.io On Behalf Of Laszlo Ersek >> Sent: Friday, August 30, 2019 5:58 AM >> To: devel@edk2.groups.io; Gao, Liming ; >> r...@edk2.groups.io; Kinney, Michael D >> >> Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration >> Phase 1 >> >> On 08/30/19 10:43, Liming Gao wrote: >>> Mike: >>> I add my comments. >>> >>>> -Original Message- >>>> From: r...@edk2.groups.io [mailto:r...@edk2.groups.io] On Behalf Of Michael >>>> D Kinney >>>> Sent: Friday, August 30, 2019 4:23 AM >>>> To: devel@edk2.groups.io; r...@edk2.groups.io >>>> Subject: [edk2-rfc] [RFC] EDK II Continuous Integration Phase 1 >>>> >>>> Hello, >>>> >>>> This is a proposal for a first step towards continuous >>>> integration for all TianoCore repositories to help >>>> improve to quality of commits and automate testing and >>>> release processes for all EDK II packages and platforms. >>>> >>>> This is based on work from a number of EDK II community >>>> members that have provide valuable input and evaluations. >>>> >>>> * Rebecca Cran Jenkins evaluation >>>> * Laszlo Ersek GitLab evaluation >>>> * Philippe Mathieu-Daudé GitLab evaluation >>>> * Sean Brogan Azure Pipelines and HBFA >>>> * Bret Barkelew Azure Pipelines and HBFA >>>> * Jiewen Yao HBFA >>>> >>>> The following link is a link to an EDK II WIKI page that >>>> contains a summary of the work to date. Please provide >>>> feedback in the EDK II mailing lists. The WIKI pages will >>>> be updated with input from the entire EDK II community. >>>> >>>>https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Continuous- >>>> Integration >>>> >>>> Proposal >>>> >>>> Phase 1 of adding continuous integration is limited to the >>>> edk2 repository. Additional repositories will be added later. >>>> >>>> The following changes are proposed: >>>> * Remove EDK II Maintainers write access to edk2 repository. >>>> Only EDK II Administrators will continue to have write >>>> access, and that should only be used to handle extraordinary >>>> events. >>>> * EDK II Maintainers use a GitHub Pull Request instead of push >>>> to commit a patch series to the edk2 repository. There are >>>> no other changes to the development and review process. The >>>> patch series is prepared in an EDK II maintainer branch with >
Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1
Can someone draw a flow chart of who does what to help clarify? 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. Are maintainers still needed to be picked up/chosen/promoted from developers? > -Original Message- > From: r...@edk2.groups.io On Behalf Of Laszlo Ersek > Sent: Friday, August 30, 2019 5:58 AM > To: devel@edk2.groups.io; Gao, Liming ; > r...@edk2.groups.io; Kinney, Michael D > > Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration > Phase 1 > > On 08/30/19 10:43, Liming Gao wrote: > > Mike: > > I add my comments. > > > >> -Original Message- > >> From: r...@edk2.groups.io [mailto:r...@edk2.groups.io] On Behalf Of Michael > >> D Kinney > >> Sent: Friday, August 30, 2019 4:23 AM > >> To: devel@edk2.groups.io; r...@edk2.groups.io > >> Subject: [edk2-rfc] [RFC] EDK II Continuous Integration Phase 1 > >> > >> Hello, > >> > >> This is a proposal for a first step towards continuous > >> integration for all TianoCore repositories to help > >> improve to quality of commits and automate testing and > >> release processes for all EDK II packages and platforms. > >> > >> This is based on work from a number of EDK II community > >> members that have provide valuable input and evaluations. > >> > >> * Rebecca Cran Jenkins evaluation > >> * Laszlo Ersek GitLab evaluation > >> * Philippe Mathieu-Daudé GitLab evaluation > >> * Sean Brogan Azure Pipelines and HBFA > >> * Bret Barkelew Azure Pipelines and HBFA > >> * Jiewen Yao HBFA > >> > >> The following link is a link to an EDK II WIKI page that > >> contains a summary of the work to date. Please provide > >> feedback in the EDK II mailing lists. The WIKI pages will > >> be updated with input from the entire EDK II community. > >> > >>https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Continuous- > >> Integration > >> > >> Proposal > >> > >> Phase 1 of adding continuous integration is limited to the > >> edk2 repository. Additional repositories will be added later. > >> > >> The following changes are proposed: > >> * Remove EDK II Maintainers write access to edk2 repository. > >> Only EDK II Administrators will continue to have write > >> access, and that should only be used to handle extraordinary > >> events. > >> * EDK II Maintainers use a GitHub Pull Request instead of push > >> to commit a patch series to the edk2 repository. There are > >> no other changes to the development and review process. The > >> patch series is prepared in an EDK II maintainer branch with > >> all commit message requirements met on each patch in the series. > > > > Will the maintainer manually provide pull request after the patch passes > > the review? > > Yes. The maintainer will prepare a local branch that is rebased to > master, and has all the mailing list feedback tags (Reviewed-by, etc) > applied. The maintainer also does all the local testing that they > usually do, just before the final "git push origin". > > Then, the final "git push origin" action is replaced by: > (1) git push personal-repo topic-branch-pr > (2) manual creation of a GitHub.com Pull Request, for the topic branch > just pushed. > > That is, the final "git push origin" action is replaced with the pushing > of a personal (ready-made) topic branch, and a GitHub.com Pull Request > against that branch. The verification and the final merging will be > performed by github. > > > Can the script scan the mail list and auto trig pull request once the patch > > gets > > Reviewed-by or Acked-by from Package maintainers? > > No, it can't. (And, at this stage, it should not.) The coordination > between submitters, maintainers, reviewers, and occasionally, stewards, > for determining when a patch series is ready to go, based on review > discussion, remains the same. > > >> * The edk2 repository only accepts Pull Requests from members > >> of the EDK II Maintainers team. Pull Requests from anyone else > >> are rejected. > >> * Run pre-commit checks using Azure Pipelines > > > > The maintainer manually trig pre-commit check or auto trig pre-commit? > > After the maintainer pushes the ready-made branch to their personal > repo, and s
