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

["Laszlo Ersek" <lersek@redhat.com>]

On 11/13/19 17:23, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> I am also reviewing a number of the PRs.
> 
> If a PR is composed of a single commit, and the commit
> message includes the reference to the Tianocore BZ, then
> the web view of the PR looks good and includes the link
> to the BZ.  A couple examples:
> 
> https://github.com/tianocore/edk2/pull/161
> https://github.com/tianocore/edk2/pull/162
> 
> The PR you referred to below is a patch series that is
> composed of 2 patches.  The individual patches do provide
> the Tianocore BZ references.  When a patch series is 
> submitted for review on the mailing list, we recommend
> a patch #0 summary that should also contain the Tianocore
> BZ references.

Right.

> 
> When a PR is opened, an initial comment can be provided.
> Perhaps the easiest way to make sure the PR is correct is
> to put the patch #0 Summary contents into this comment
> when the PR is created for a patch series.

Good idea -- combined with the above, that would place the BZ link in
the initial email notification too.

(Including the BZ number in the title of the PR would be even more
helpful, as virtually any email client should allow the user to search
email subjects for a BZ number.)

> This also
> provides another place the patch #0 summary contents are
> recorded since those contents are never recorded in the
> git history.
> 
> Unfortunately, the specific PR you referenced here did not
> have complete patch #0 summary contents when it was reviewed
> on the mailing list. 
> 
> https://edk2.groups.io/g/devel/message/50323
> 
> I agree that the Tianocore BZ should be updated with a 
> link to the PR.  This is not really an extra step.  When
> a BZ is closed, the BZ should be updated with the range of
> commits that were pushed.  Providing a link to the PR that
> was merged provides the equivalent information.  The SHA
> hash range of the commits can be provided as well in the
> same comment that closes the BZ.

Slightly disagree about "equivalent" -- equivalent if the user is online
and can follow the link from Bugzilla to GitHub (or else the user has
the github email notifications for that PR already cached locally). If
it's not a huge burden, I'd like to see both pieces of information
included in the BZ comment (commit range and PR link).

Thank you!
Laszlo


>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, November 13, 2019 12:57 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Sean Brogan
>> <sean.brogan@microsoft.com>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>; Gao, Liming
>> <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
>> CI is now active on edk2/master
>>
>> Hi again,
>>
>> (+Ray)
>>
>> On 11/12/19 03:55, Michael D Kinney wrote:
>>> EDK II Maintainers,
>>>
>>> EDK II CI Phase 1 feature is now active on
>> edk2/master.
>>>
>>> Please use a GitHub pull request from a branch in a
>> personal fork of
>>> the edk2 repository with a 'push' label to request a
>> set of patches to
>>> be pushed to edk2/master.  The GitHub PR replaces the
>> 'git push'
>>> operation currently used to commit changes to
>> edk2/master.
>>>
>>> You will need to configure your notifications from
>> the edk2 repository
>>> to make sure you receive email notifications when the
>> checks against
>>> the GitHub PR passes or fails.
>>>
>>> If you submit a GitHub Pull Request without the
>> 'push'
>>> label, then the CI checks are run and the results are
>> generated.
>>>
>>> Please let us know if there are any questions about
>> this change in the
>>> development process.
>>
>> now that a few PRs have been merged "in production",
>> using the above procedure, I'd like to propose an
>> addition.
>>
>> I've received a number of PR notifications, and I
>> generally have no idea how to associate them with what
>> happens on the list. My proposal is supposed to improve
>> on that.
>>
>> Note that, I have always suggested / requested
>> including links, pointing into the mailing list
>> archive, in Bugzilla tickets for which new versions of
>> patch sets have been posted. This (brief) proposal is a
>> natural continuation / extension of that idea.
>>
>> - When opening a GitHub PR, as described in the above
>> procedure, please
>> *always* include a reference in the PR's title to the
>> associated TianoCore bugzilla ticket, if there is one.
>>
>> - Similarly, please add a link, pointing to the GitHub
>> PR, to the bugzilla ticket.
>>
>> For example, if I look at the following email:
>>
>>   [tianocore/edk2] Cpu/remove xd (#164)
>>
>> it tells me basically *nothing*. And if I open
>>
>>   https://github.com/tianocore/edk2/pull/164
>>
>> in my web browser, it still tells me nothing.
>>
>> The PR title should include a reference to
>> TianoCore#2329, and
>> TianoCore#2329 should include a reference to
>> <https://github.com/tianocore/edk2/pull/164>.
>>
>> Yes, this is more work than before. It is necessary
>> because now we have *more artifacts* related to pushing
>> (merging) a patch series. Those artifacts should not
>> just hang in the air.
>>
>> Ray: now that the series has been merged, can you
>> please reference the GitHub PR, and the resultant
>> commit range, in TianoCore#2329? Also, can you please
>> close the bugzilla as fixed?
>>
>> Thanks,
>> Laszlo
>