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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#51448): https://edk2.groups.io/g/devel/message/51448 Mute This Topic: https://groups.io/mt/53725670/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
