EDK II Maintainers - EDK II CI is now active on edk2/master

EDK II Maintainers - EDK II CI is now active on edk2/master

[Michael D Kinney]

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.

Best regards,

Mike

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

[Laszlo Ersek]

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.

Awesome!

> 
> 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.

Can you please provide instructions for reaching those notification
settings?

(If there is a single "settings" URL that I have to open in my browser,
while being logged in to my GitHub account, then sharing that URL here
would be ideal.)

Thanks!
Laszlo

> 
> 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.
> 
> Best regards,
> 
> Mike
> 
> 
> 

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

[Michael D Kinney]

Hi Laszlo,

The setting that is required is 'Watch' on the edk2 repo.
Navigate to the main page for the tianocore edk2 repo:

    https://github.com/tianocore/edk2

At the top of the page, there is a drop down called 'Watch'.
Select 'Watching - Be notified of all conversations.'

GitHub documentation on this feature:

https://help.github.com/en/github/receiving-notifications-about-activity-on-github/about-notifications#watching-notifications

Best regards,

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, November 12, 2019 12:56 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>
> Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
> CI is now active on edk2/master
> 
> 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.
> 
> Awesome!
> 
> >
> > 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.
> 
> Can you please provide instructions for reaching those
> notification settings?
> 
> (If there is a single "settings" URL that I have to
> open in my browser, while being logged in to my GitHub
> account, then sharing that URL here would be ideal.)
> 
> Thanks!
> Laszlo
> 
> >
> > 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.
> >
> > Best regards,
> >
> > Mike
> >
> > 
> >

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

[Michael D Kinney]

GitHub link with screen shots to enable 'Watch' feature.

https://help.github.com/en/github/receiving-notifications-about-activity-on-github/watching-and-unwatching-repositories

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Tuesday, November 12, 2019 11:51 AM
> To: Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Sean Brogan
> <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] EDK II Maintainers - EDK II
> CI is now active on edk2/master
> 
> Hi Laszlo,
> 
> The setting that is required is 'Watch' on the edk2
> repo.
> Navigate to the main page for the tianocore edk2 repo:
> 
>     https://github.com/tianocore/edk2
> 
> At the top of the page, there is a drop down called
> 'Watch'.
> Select 'Watching - Be notified of all conversations.'
> 
> GitHub documentation on this feature:
> 
> https://help.github.com/en/github/receiving-
> notifications-about-activity-on-github/about-
> notifications#watching-notifications
> 
> Best regards,
> 
> Mike
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Tuesday, November 12, 2019 12:56 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>
> > Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
> CI is now active
> > on edk2/master
> >
> > 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.
> >
> > Awesome!
> >
> > >
> > > 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.
> >
> > Can you please provide instructions for reaching
> those notification
> > settings?
> >
> > (If there is a single "settings" URL that I have to
> open in my
> > browser, while being logged in to my GitHub account,
> then sharing that
> > URL here would be ideal.)
> >
> > Thanks!
> > Laszlo
> >
> > >
> > > 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.
> > >
> > > Best regards,
> > >
> > > Mike
> > >
> > > 
> > >

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

[Laszlo Ersek]

On 11/12/19 20:50, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> The setting that is required is 'Watch' on the edk2 repo.
> Navigate to the main page for the tianocore edk2 repo:
> 
>     https://github.com/tianocore/edk2
> 
> At the top of the page, there is a drop down called 'Watch'.
> Select 'Watching - Be notified of all conversations.'

Thank you; it looks like I had this set already. (I sort of suspected
that, but wanted to make sure I had the right config in place.)

Cheers!
Laszlo

> GitHub documentation on this feature:
> 
> https://help.github.com/en/github/receiving-notifications-about-activity-on-github/about-notifications#watching-notifications
> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, November 12, 2019 12:56 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>
>> Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
>> CI is now active on edk2/master
>>
>> 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.
>>
>> Awesome!
>>
>>>
>>> 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.
>>
>> Can you please provide instructions for reaching those
>> notification settings?
>>
>> (If there is a single "settings" URL that I have to
>> open in my browser, while being logged in to my GitHub
>> account, then sharing that URL here would be ideal.)
>>
>> Thanks!
>> Laszlo
>>
>>>
>>> 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.
>>>
>>> Best regards,
>>>
>>> Mike
>>>
>>> 
>>>
> 

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

[Laszlo Ersek]

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

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

[Michael D Kinney]

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.

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.  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.

Thanks,

Mike

> -----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

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

[Laszlo Ersek]

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
> 

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

[Laszlo Ersek]

Hi Mike,

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.

I think this workflow feature has been working well; thank you (and
everyone else involved) again for implementing it.

Having read a good number of the emails sent out by github.com about
such Pull Requests, I'd like to request a new feature, or at least to
suggest a usage convention.

The presence of the "push" label decides whether the PR is actually
meant for master, or if it's a personal build (usually employed as a
sanity check before posting a series to the list for review).

This difference is important, but there is no sign of the "push" label
(or of its absence) in the emails that github.com generates, as far as I
can tell. Right now I can only tell the difference from the final
mergify bot comment that says

- "Merged #197 into master."

versus

- "All checks passed. Auto close personal build."

Can we please
- either tweak github.com to include any labels that are tacked to the
PR, in the notification emails,
- or adopt a convention where "personal build" PRs are required to state
that fact in the PR title?

Thanks!
Laszlo

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

[Michael D Kinney]

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?

A PR may be created initially without the 'push' label.
The EDK II Maintainer can resolve any issues in that PR
using forced pushes to restart the checks with the 
issues resolved.  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.  In this
use case what email notifications would you like to see?

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, November 26, 2019 12:23 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>
> Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
> CI is now active on edk2/master
> 
> Hi Mike,
> 
> 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.
> 
> I think this workflow feature has been working well;
> thank you (and everyone else involved) again for
> implementing it.
> 
> Having read a good number of the emails sent out by
> github.com about such Pull Requests, I'd like to
> request a new feature, or at least to suggest a usage
> convention.
> 
> The presence of the "push" label decides whether the PR
> is actually meant for master, or if it's a personal
> build (usually employed as a sanity check before
> posting a series to the list for review).
> 
> This difference is important, but there is no sign of
> the "push" label (or of its absence) in the emails that
> github.com generates, as far as I can tell. Right now I
> can only tell the difference from the final mergify bot
> comment that says
> 
> - "Merged #197 into master."
> 
> versus
> 
> - "All checks passed. Auto close personal build."
> 
> Can we please
> - either tweak github.com to include any labels that
> are tacked to the PR, in the notification emails,
> - or adopt a convention where "personal build" PRs are
> required to state that fact in the PR title?
> 
> Thanks!
> Laszlo

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

[Laszlo Ersek]

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 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 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.

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.)

> 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".)

Thanks!
Laszlo

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

[Michael D Kinney]

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

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

[Laszlo Ersek]

On 12/02/19 20:55, Kinney, Michael D wrote:

> 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?

Thank you for explaining.

In this case, I think it would be helpful if, whenever another email
notification were sent out about a PR, the subject of that email
contained an auto-generated part that advertized the push label's
presence on the PR, at that time.

OTOH... It's also possible that I'm approaching this wrong. After all,
before we adopted github.com for pushing, a maintainer would just go
ahead and push post-review (with no *automatic* email notification about
the fact), and then we'd expect that maintainer to report back under the
patch review thread ("pushed as commit range ..."), and/or close the BZ
ticket with a similar comment. (And then there would be an
auto-generated bugzilla email about that.)

My point being, maybe I shouldn't even *read* these github.com
notifications at all. I'm not sure if they give me useful information.
(When the PR is ultimately merged, we *still* require the above kind of
closing comment in Bugzilla, from the maintainer -- thus the originally
needed information is still provided, just like before.)

It's just that, *if* I attempt to read the github.com emails, *then*
they confuse me (for example because they don't expose the push label).
If we consider this specific kind of PR that we have adopted for edk2 a
(practically) "maintainer-internal", mechanical, action, then I guess I
might as well want to unsubscribe from those notifications. After all
I'm still subscribed to BZ emails, and I'll see the resultant commit
range noted there (through the comment that the assignee adds manually,
when they close the BZ).

Is it possible for a tianocore group member to "unwatch" PRs?

Thanks!
Laszlo

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

[Michael D Kinney]

Hi Laszlo,

I can think of ways we may be able to get the label 
information into the body of the email in a PR comment.
I will have to see if there is anyway to adjust the subject
of the email.  The subject appears to always match the title
of the PR.  That title is editable, but may only be editable
using GitHub APIs.  Mergify does not support that feature 
today.

If you look at the following file, you will see the current
Mergify rules.

https://github.com/tianocore/edk2/blob/master/.mergify/config.yml

If I look at the 3rd rule for merge conflicts, I could split
that out into 2 rules.  One for push label set and one for
push label not set and adjust the message generated to state
if the conflict is on a personal build or a push request.

Would small adjustments like this in the body of the email
notifications help?  If we make the generated messages consistent
email filters on the content of the email body can be used 
instead of email filters on the email subject.

Each developer can enable/disable the 'Watch' feature on the
edk2 repo.  That is a developer setting, not a team setting.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, December 3, 2019 12:56 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 12/02/19 20:55, Kinney, Michael D wrote:
> 
> > 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?
> 
> Thank you for explaining.
> 
> In this case, I think it would be helpful if, whenever
> another email
> notification were sent out about a PR, the subject of
> that email
> contained an auto-generated part that advertized the
> push label's
> presence on the PR, at that time.
> 
> OTOH... It's also possible that I'm approaching this
> wrong. After all,
> before we adopted github.com for pushing, a maintainer
> would just go
> ahead and push post-review (with no *automatic* email
> notification about
> the fact), and then we'd expect that maintainer to
> report back under the
> patch review thread ("pushed as commit range ..."),
> and/or close the BZ
> ticket with a similar comment. (And then there would be
> an
> auto-generated bugzilla email about that.)
> 
> My point being, maybe I shouldn't even *read* these
> github.com
> notifications at all. I'm not sure if they give me
> useful information.
> (When the PR is ultimately merged, we *still* require
> the above kind of
> closing comment in Bugzilla, from the maintainer --
> thus the originally
> needed information is still provided, just like
> before.)
> 
> It's just that, *if* I attempt to read the github.com
> emails, *then*
> they confuse me (for example because they don't expose
> the push label).
> If we consider this specific kind of PR that we have
> adopted for edk2 a
> (practically) "maintainer-internal", mechanical,
> action, then I guess I
> might as well want to unsubscribe from those
> notifications. After all
> I'm still subscribed to BZ emails, and I'll see the
> resultant commit
> range noted there (through the comment that the
> assignee adds manually,
> when they close the BZ).
> 
> Is it possible for a tianocore group member to
> "unwatch" PRs?
> 
> Thanks!
> Laszlo

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

[Laszlo Ersek]

On 12/03/19 18:07, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> I can think of ways we may be able to get the label 
> information into the body of the email in a PR comment.
> I will have to see if there is anyway to adjust the subject
> of the email.  The subject appears to always match the title
> of the PR.  That title is editable, but may only be editable
> using GitHub APIs.  Mergify does not support that feature 
> today.
> 
> If you look at the following file, you will see the current
> Mergify rules.
> 
> https://github.com/tianocore/edk2/blob/master/.mergify/config.yml
> 
> If I look at the 3rd rule for merge conflicts, I could split
> that out into 2 rules.  One for push label set and one for
> push label not set and adjust the message generated to state
> if the conflict is on a personal build or a push request.
> 
> Would small adjustments like this in the body of the email
> notifications help?  If we make the generated messages consistent
> email filters on the content of the email body can be used 
> instead of email filters on the email subject.

Sounds great, thank you. I've checked the mail filtering solution that's
available to me, and it appears capable of scanning email bodies.

I suggest picking a GUID for the message body, in best UEFI style :) ,
for representing the *absence* of the "push" label. I generally prefer
watching all kinds of notifications about the edk2 repo, *except* the
personal CI builds.

> Each developer can enable/disable the 'Watch' feature on the
> edk2 repo.  That is a developer setting, not a team setting.

Thanks!
Laszlo

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

[Laszlo Ersek]

Hi Mike,

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.

I don't know where I can file RFEs for the CI system.

For now, I've filed:

https://bugzilla.tianocore.org/show_bug.cgi?id=2406

The request is to allow subsystem maintainers to opt out of, or waive,
PatchCheck.py failures. The commit message line length checks in
PatchCheck.py make sense as a default, but there are valid cases when
they should be turned off. Please see details in the BZ.

Thanks!
Laszlo

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

[Laszlo Ersek]

On 12/06/19 12:02, Laszlo Ersek wrote:
> Hi Mike,
> 
> 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.
> 
> I don't know where I can file RFEs for the CI system.
> 
> For now, I've filed:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2406
> 
> The request is to allow subsystem maintainers to opt out of, or waive,
> PatchCheck.py failures. The commit message line length checks in
> PatchCheck.py make sense as a default, but there are valid cases when
> they should be turned off. Please see details in the BZ.

Forgot to CC Phil on this message, sorry about that; doing it now.
Laszlo

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

[Philippe Mathieu-Daudé]

Hi Michael,

On 11/12/19 3:55 AM, 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.

I have 2 requests:

1/ Is it possible to have the Mergify bot use the merge request author 
name/email as GIT_COMMITTER_[NAME/EMAIL] instead of throwing away this 
information from the git history?

Before we could directly use this information to email the maintainer 
who merged the patch in.
Currently I can't find an easy way... I've to dig thru the mailing list 
archives for the last thread, figure out who could have merge this, 
check the different maintainers GitHub repository...

The new way might be using this filter (which is not listed by default):
https://github.com/tianocore/edk2/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged

Then go thru the last ones, but this doesn't scale in few months to look 
at old merges.

2/ Can the Mergify bot send a mail to the list to notify a patch got merged?

For example going thru my backlog I was going to review this series:
https://edk2.groups.io/g/devel/message/52613
But it is already merged... The series subject is "Microcode related 
optimizations" and when I searched for it first with the GitHub MR 
filter from 1/, I couldn't find it. Later I figured it got merged with 
another subject "Mpinitlib opt push 1".

One way to simplify Mergify to send email, is to ask maintainers to put 
the series cover (or each patches) URL in the GitHub merge request 
description, or the email Message ID(s). Since we are switching the a 
mostly HTTP workflow, using URLs is probably recommended.

Thanks, and happy new year! :)

Phil.

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

[Michael D Kinney]

Hi Phil,

I am curious why the GIT committer information is so important.

Before CI, for any given package, the committer can be the
primary maintainer, the backup maintainer, or a steward.
The maintainer that actually does the commit is following the 
process and the rules for doing a push, but may not have been
involved in the code change, reviews, or testing.

To me, the critical information is the Author, Reviewed-by,
Acked-by, and Tested-by tags, which I believe are all correct
when Mergify bot does the commit.

What is the use case I am missing here?

Thanks,

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Philippe Mathieu-Daudé
> Sent: Thursday, January 2, 2020 6:42 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>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
> CI is now active on edk2/master
> 
> Hi Michael,
> 
> On 11/12/19 3:55 AM, 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.
> 
> I have 2 requests:
> 
> 1/ Is it possible to have the Mergify bot use the merge
> request author
> name/email as GIT_COMMITTER_[NAME/EMAIL] instead of
> throwing away this
> information from the git history?
> 
> Before we could directly use this information to email
> the maintainer
> who merged the patch in.
> Currently I can't find an easy way... I've to dig thru
> the mailing list
> archives for the last thread, figure out who could have
> merge this,
> check the different maintainers GitHub repository...
> 
> The new way might be using this filter (which is not
> listed by default):
> https://github.com/tianocore/edk2/pulls?utf8=%E2%9C%93&
> q=is%3Apr+is%3Amerged
> 
> Then go thru the last ones, but this doesn't scale in
> few months to look
> at old merges.
> 
> 2/ Can the Mergify bot send a mail to the list to
> notify a patch got merged?
> 
> For example going thru my backlog I was going to review
> this series:
> https://edk2.groups.io/g/devel/message/52613
> But it is already merged... The series subject is
> "Microcode related
> optimizations" and when I searched for it first with
> the GitHub MR
> filter from 1/, I couldn't find it. Later I figured it
> got merged with
> another subject "Mpinitlib opt push 1".
> 
> One way to simplify Mergify to send email, is to ask
> maintainers to put
> the series cover (or each patches) URL in the GitHub
> merge request
> description, or the email Message ID(s). Since we are
> switching the a
> mostly HTTP workflow, using URLs is probably
> recommended.
> 
> Thanks, and happy new year! :)
> 
> Phil.
> 
> 
> 

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

[Philippe Mathieu-Daudé]

On 1/2/20 7:36 PM, Kinney, Michael D wrote:
> Hi Phil,
> 
> I am curious why the GIT committer information is so important.
> 
> Before CI, for any given package, the committer can be the
> primary maintainer, the backup maintainer, or a steward.
> The maintainer that actually does the commit is following the
> process and the rules for doing a push, but may not have been
> involved in the code change, reviews, or testing.
> 
> To me, the critical information is the Author, Reviewed-by,
> Acked-by, and Tested-by tags, which I believe are all correct
> when Mergify bot does the commit.

Yes, they are correct.

> 
> What is the use case I am missing here?

I guess I'm probably too rigid, custom to process all merge information 
in my gitk window.
If one day I've too look at Mergify I'll see if it is possible to 
properly set the GIT_COMMITTER_[NAME/EMAIL] env vars before merging.

Sorry for the noise,

Phil.

>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On
>> Behalf Of Philippe Mathieu-Daudé
>> Sent: Thursday, January 2, 2020 6:42 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>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
>> CI is now active on edk2/master
>>
>> Hi Michael,
>>
>> On 11/12/19 3:55 AM, 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.
>>
>> I have 2 requests:
>>
>> 1/ Is it possible to have the Mergify bot use the merge
>> request author
>> name/email as GIT_COMMITTER_[NAME/EMAIL] instead of
>> throwing away this
>> information from the git history?
>>
>> Before we could directly use this information to email
>> the maintainer
>> who merged the patch in.
>> Currently I can't find an easy way... I've to dig thru
>> the mailing list
>> archives for the last thread, figure out who could have
>> merge this,
>> check the different maintainers GitHub repository...
>>
>> The new way might be using this filter (which is not
>> listed by default):
>> https://github.com/tianocore/edk2/pulls?utf8=%E2%9C%93&
>> q=is%3Apr+is%3Amerged
>>
>> Then go thru the last ones, but this doesn't scale in
>> few months to look
>> at old merges.
>>
>> 2/ Can the Mergify bot send a mail to the list to
>> notify a patch got merged?
>>
>> For example going thru my backlog I was going to review
>> this series:
>> https://edk2.groups.io/g/devel/message/52613
>> But it is already merged... The series subject is
>> "Microcode related
>> optimizations" and when I searched for it first with
>> the GitHub MR
>> filter from 1/, I couldn't find it. Later I figured it
>> got merged with
>> another subject "Mpinitlib opt push 1".
>>
>> One way to simplify Mergify to send email, is to ask
>> maintainers to put
>> the series cover (or each patches) URL in the GitHub
>> merge request
>> description, or the email Message ID(s). Since we are
>> switching the a
>> mostly HTTP workflow, using URLs is probably
>> recommended.
>>
>> Thanks, and happy new year! :)
>>
>> Phil.
>>
>>
>> 
> 

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

[Laszlo Ersek]

On 01/02/20 15:42, Philippe Mathieu-Daudé wrote:

> 1/ Is it possible to have the Mergify bot use the merge request author
> name/email as GIT_COMMITTER_[NAME/EMAIL] instead of throwing away this
> information from the git history?

I noticed that too, but I thought that having a robot rather than a
human in the committer meta-datum was tolerable. (Not great, but
acceptable.)

The authorship meta-datum is still correct, to my understanding. If
there's a problem later with a commit, I'd probably email the author (CC
list), not the committer.

> 2/ Can the Mergify bot send a mail to the list to notify a patch got
> merged?
> 
> For example going thru my backlog I was going to review this series:
> https://edk2.groups.io/g/devel/message/52613
> But it is already merged... The series subject is "Microcode related
> optimizations" and when I searched for it first with the GitHub MR
> filter from 1/, I couldn't find it. Later I figured it got merged with
> another subject "Mpinitlib opt push 1".
> 
> One way to simplify Mergify to send email, is to ask maintainers to put
> the series cover (or each patches) URL in the GitHub merge request
> description, or the email Message ID(s). Since we are switching the a
> mostly HTTP workflow, using URLs is probably recommended.

Including the cover letter contents and/or mailing list reference in the
PR description is already recommended practice, to my understanding. The
related tianocore BZ should be noted in the PR description too. AIUI,
it's also recommended to name the PR in accordance with the series cover
letter's subject line. Unfortunately, maintainers don't seem to be
following these recommendations.

Regarding an email notification about a merge, I have two comments:

- Personally, I always follow up with a manual message to the list, once
a series is merged, pointing out the new commit range. It's not a huge
burden.

- Github generates emails about PR actions. Unfortunately, the current
scheme for a merge ("Merged #263 into master") is really lacking. It
cannot be easily filtered for (you have to check the message body to see
it's a "merge"), plus the resultant commit range (-> master branch
advance) is not communicated. I don't know if this would be best
remedied in the general github.com website code, or in the mergify bot code.

No tooling will ever provide enough information for the community so
long as maintainers are unwilling to spend time on administrative
actions. The attraction of github.com for the grand public is not that
github.com implements these administrative actions itself, un-burdening
developers; instead, the attraction is that github.com pretends these
actions are not important at all, and so nobody should care about them.
You can't really make a tool care if maintainers don't care.

Thanks
Laszlo

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

[Laszlo Ersek]

Hi Mike,

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.

I had filed

  https://bugzilla.tianocore.org/show_bug.cgi?id=2406

a month ago, about PatchCheck.py being too strict, and blocking pull requests unjustifiedly.

Now I realize I can't even review the results (= the PatchCheck.py error messages) on github.com and/or on Azure Pipelines:

  https://github.com/tianocore/edk2/pull/272/checks?check_run_id=375969977
  https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-8b13-7def1f19d478/_build/results?buildId=3456

Am I looking in the wrong place perhaps?

Can we please add a manual PatchCheck.py override on github.com for those maintainers that are permitted to set the "push" label already?

TianoCore#2406 doesn't seem to be getting any attention, and it is blocking actual development work.

Thanks
Laszlo

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

[Michael D Kinney]

Laszlo,

Sorry for the delay in getting to these PactchCheck.py updates.
I know it is frustrating to be blocked by these types of issues.
I try to look at 2406 tomorrow.  I think the manual override is
potentially more complex to implement than the requested changed
to PatchCheck.py.

You were on the correct link.  If you clicked one more level on
"Bash exited with code '255'" link on that web page, it takes you
to the patch check log that shows 3 errors for commit message
subject lines being too long.

https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=3459&view=logs&j=12f1170f-54f2-53f3-20dd-22fc7dff55f9&t=9c939e41-62c2-5605-5e05-fc3554afc9f5&l=149

Best regards,

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, January 6, 2020 9:29 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>
> Subject: Re: [edk2-devel] EDK II Maintainers - EDK II
> CI is now active on edk2/master
> 
> Hi Mike,
> 
> 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.
> 
> I had filed
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=2406
> 
> a month ago, about PatchCheck.py being too strict, and
> blocking pull requests unjustifiedly.
> 
> Now I realize I can't even review the results (= the
> PatchCheck.py error messages) on github.com and/or on
> Azure Pipelines:
> 
> 
> https://github.com/tianocore/edk2/pull/272/checks?check
> _run_id=375969977
>   https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-
> 8b13-7def1f19d478/_build/results?buildId=3456
> 
> Am I looking in the wrong place perhaps?
> 
> Can we please add a manual PatchCheck.py override on
> github.com for those maintainers that are permitted to
> set the "push" label already?
> 
> TianoCore#2406 doesn't seem to be getting any
> attention, and it is blocking actual development work.
> 
> Thanks
> Laszlo

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

[Laszlo Ersek]

Hello Mike,

On 01/06/20 19:17, Kinney, Michael D wrote:
> Laszlo,
> 
> Sorry for the delay in getting to these PactchCheck.py updates.
> I know it is frustrating to be blocked by these types of issues.
> I try to look at 2406 tomorrow.

Thanks!

> I think the manual override is
> potentially more complex to implement than the requested changed
> to PatchCheck.py.

OK.

> 
> You were on the correct link.  If you clicked one more level on
> "Bash exited with code '255'" link on that web page, it takes you
> to the patch check log that shows 3 errors for commit message
> subject lines being too long.
> 
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=3459&view=logs&j=12f1170f-54f2-53f3-20dd-22fc7dff55f9&t=9c939e41-62c2-5605-5e05-fc3554afc9f5&l=149

Thanks for this info as well. I have two comments, one specific and one
general ("rambling").

(1) The specific comment: the above link does not work for me. I get:

  An unexpected error has occurred within this region of the page.
  You can try reloading this component or refreshing the entire page.

with two buttons:

  [Refresh page] [Reload component]

Neither helps -- the result is the same. Then, if I click "Show more
info", I get:

  Error: 'block' member of ScrollIntoViewOptions 'center' is not a valid
  value for enumeration ScrollLogicalPosition.
  Stack
  in i in t in div in t in li ...


The result is the same if I start with the link from yesterday:

https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-8b13-7def1f19d478/_build/results?buildId=3456

and then indeed click the "Bash exited with code '255'" link.


Worse, even the github.com link that I posted myself yesterday:

https://github.com/tianocore/edk2/pull/272/checks?check_run_id=375969977

has stopped working now. Apparently failed CI runs are superseded by
successful ones, and we can no longer refer to historic failures. This
is not very helpful (in particular for the CI system's developers, I'd
think!).


(2) The general comment. In the old [X]HTML days, a button looked like a
button, and a link looked like a link. Today, on the Modern Web (TM),
visual cues as to whether a UI element is "active" or not, are
unpredictable. To me anyway. There are too many colors, and the active
elements frequently lack a consistent trait -- even within a single web
page -- that advertises "I'm active, click me", without the user
hovering over them with the mouse pointer.

My question here is whether this is
- just me,
- my background (~ textual terminals),
- my generation (Gen X, let's say).

I feel like UI design has fallen off a cliff in recent years.

(Bonus offense: text that cannot be selected for copy & paste. For
example, I cannot highlight the "Bash exited with code '255'" link, for
copying that caption into this email. I have to re-type it manually.)

Thanks
Laszlo

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

[Laszlo Ersek]

Hi Mike,

On 01/06/20 19:17, Kinney, Michael D wrote:
> Laszlo,
> 
> Sorry for the delay in getting to these PactchCheck.py updates.
> I know it is frustrating to be blocked by these types of issues.
> I try to look at 2406 tomorrow.  I think the manual override is
> potentially more complex to implement than the requested changed
> to PatchCheck.py.

can we please downgrade the commit message width check from "error" to
"warning"?

It is crucial that I be able to quote edk2 *source code* in some commit
messages. And edk2 uses very long lines in C source code.

Thanks
Laszlo

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

[Michael D Kinney]

Yes.  That request is already captured in the BZ.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 9, 2020 1:31 PM
> 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
> 
> Hi Mike,
> 
> On 01/06/20 19:17, Kinney, Michael D wrote:
> > Laszlo,
> >
> > Sorry for the delay in getting to these
> PactchCheck.py updates.
> > I know it is frustrating to be blocked by these types
> of issues.
> > I try to look at 2406 tomorrow.  I think the manual
> override is
> > potentially more complex to implement than the
> requested changed
> > to PatchCheck.py.
> 
> can we please downgrade the commit message width check
> from "error" to
> "warning"?
> 
> It is crucial that I be able to quote edk2 *source
> code* in some commit
> messages. And edk2 uses very long lines in C source
> code.
> 
> Thanks
> Laszlo

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

[Laszlo Ersek]

On 01/09/20 22:37, Michael D Kinney wrote:
> Yes.  That request is already captured in the BZ.

My bad -- I ran into the issue last night again, but had forgotten about
the BZ's particulars by then. Apologies!

Thanks
Laszlo


>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, January 9, 2020 1:31 PM
>> 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
>>
>> Hi Mike,
>>
>> On 01/06/20 19:17, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> Sorry for the delay in getting to these
>> PactchCheck.py updates.
>>> I know it is frustrating to be blocked by these types
>> of issues.
>>> I try to look at 2406 tomorrow.  I think the manual
>> override is
>>> potentially more complex to implement than the
>> requested changed
>>> to PatchCheck.py.
>>
>> can we please downgrade the commit message width check
>> from "error" to
>> "warning"?
>>
>> It is crucial that I be able to quote edk2 *source
>> code* in some commit
>> messages. And edk2 uses very long lines in C source
>> code.
>>
>> Thanks
>> Laszlo
> 
> 
> 
> 

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

[Laszlo Ersek]

On 11/12/19 03:55, Michael D Kinney wrote:

> Please let us know if there are any questions about this
> change in the development process.

What is the recommended way to report issues with the mergify (not CI)
infrastructure?

Mergify hasn't been picking up ready-to-merge branches (with CI passed
and the "push" label set).

  https://github.com/tianocore/edk2/pull/427
  https://github.com/tianocore/edk2/pull/428
  https://github.com/tianocore/edk2/pull/430

I've filed a ticket with github (#592107) and emailed
<support@mergify.io> too. But, the github ticket number doesn't seem
useful (there is not a "case" that I could overview, or share with
others, using the ticket number), and mergify.io don't seem to have
generated even a ticket number for me.

Both github.com and mergify.io claim to have 100% service level at the
moment:

- https://www.githubstatus.com/

  "All Systems Operational"

- https://www.notion.so/Mergify-Status-Page-7803a762235d4ee6bed1f9976d17bd83

  "Dashboard Operational", "Engine Operational"

In the absence of github / mergify feedback, it would be nice if at
least stewards could manually merge such topic branches that have passed
CI.

Thanks
Laszlo