From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web09.6779.1575316522330284434 for ; Mon, 02 Dec 2019 11:55:22 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: michael.d.kinney@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Dec 2019 11:55:22 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,270,1571727600"; d="scan'208";a="208204978" Received: from orsmsx103.amr.corp.intel.com ([10.22.225.130]) by fmsmga008.fm.intel.com with ESMTP; 02 Dec 2019 11:55:21 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.200]) by ORSMSX103.amr.corp.intel.com ([169.254.5.179]) with mapi id 14.03.0439.000; Mon, 2 Dec 2019 11:55:21 -0800 From: "Michael D Kinney" To: Laszlo Ersek , "devel@edk2.groups.io" , Sean Brogan , "Bret Barkelew" , "Gao, Liming" , "Kinney, Michael D" Subject: Re: [edk2-devel] EDK II Maintainers - EDK II CI is now active on edk2/master Thread-Topic: [edk2-devel] EDK II Maintainers - EDK II CI is now active on edk2/master Thread-Index: AdWY/xjw4dZ1iskvSlK5J8Hop+ZpewLdrbAAADbisuAANUr3gADIaoHw Date: Mon, 2 Dec 2019 19:55:20 +0000 Message-ID: References: <545f1da9-a998-1095-57f9-085cbd535596@redhat.com> <184dbbde-0916-7e09-476c-de700cc57dc5@redhat.com> In-Reply-To: <184dbbde-0916-7e09-476c-de700cc57dc5@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, Comments below. Mike > -----Original Message----- > From: Laszlo Ersek > Sent: Thursday, November 28, 2019 4:01 AM > To: Kinney, Michael D ; > devel@edk2.groups.io; Sean Brogan > ; Bret Barkelew > ; Gao, Liming > > Subject: Re: [edk2-devel] EDK II Maintainers - EDK II > CI is now active on edk2/master >=20 > 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? >=20 > 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). >=20 > > A PR may be created initially without the 'push' > label. >=20 > Yes, I noticed that when I first experimented with the > feature. I found it a bit surprising (but not a > problem). >=20 > > The EDK II Maintainer can resolve any issues in that > PR using forced > > pushes to restart the checks with the issues > resolved. >=20 > Yes, this is valid for a personal CI build. (Keep > trying until it passes.) >=20 > > 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. >=20 > 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. >=20 > 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. >=20 > 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 >=20 > - either be minimal; implemented, and also reported > back to the list, by the maintainer, > - or more extensive, and reposted by the original > contributor. Agree. >=20 > 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=20 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=20 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=20 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-Pr= ocess#the-maintainer-process-for-the-edk-ii-project >=20 > > In this use case what email notifications would you > like to see? >=20 > 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. >=20 > (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=20 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=20 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=20 a PR without the 'push' label to a PR with a 'push' label? >=20 > Thanks! > Laszlo