Re: Github PR label automation
Correct, it has no way to know if something has been put into ready to merge deliberately despite it failing checks etc so it won't mess with removing the label. Mark On Wed, Feb 12, 2020 at 10:39 AM Dr. Matthias St. Pierre wrote: > > > check. It will not move to 'ready to merge' state automatically > > unless (or until) all CI passes. (I'll do a PR for the tool with that > > change shortly). > > If the does not automatically remove the "ready to merge" label, but only > refrains from setting it automatically, that's a good compromise I can live > with. > > Matthias >
RE: Github PR label automation
> check. It will not move to 'ready to merge' state automatically > unless (or until) all CI passes. (I'll do a PR for the tool with that > change shortly). If the does not automatically remove the "ready to merge" label, but only refrains from setting it automatically, that's a good compromise I can live with. Matthias
Re: Github PR label automation
I thought about it some more and Kurt is right. Something shouldn't be in "Ready to Merge" unless it's actually ready to merge. For example 10993. This PR shouldn't be ever automatically moved to ready to merge because it failed CI. A human has determined it is ready to merge and applied the label. So "ready to merge" really ought to mean that. So I've updated the live script that's running to do that check. It will not move to 'ready to merge' state automatically unless (or until) all CI passes. (I'll do a PR for the tool with that change shortly). Mark On Wed, Feb 12, 2020 at 8:49 AM Dr. Matthias St. Pierre wrote: > > > > Does it also check that the CI says that everything is OK? > > > > Do we want it to? I assumed that Approval: done was not being applied > > unless tests past (but looking that's not always the case). Can we > > assume that something in "approval: ready to merge" but that failed CI > > won't get merged? Otherwise I could add a CI check on the move to > > "ready to merge". > > It does not check and I don't think it should attempt to do it. The only task > of > this script should be to automatically promote the [approval: done] state to > [approval: ready to merge] after the grace period has expired, because humans > tend to forget that. Anything beyond that, in particular an attempt to judge > whether the approval is valid or not, and whether a CI failure can be ignored > or > not, should be left to humans. > > Matthias >
RE: Github PR label automation
> > Does it also check that the CI says that everything is OK? > > Do we want it to? I assumed that Approval: done was not being applied > unless tests past (but looking that's not always the case). Can we > assume that something in "approval: ready to merge" but that failed CI > won't get merged? Otherwise I could add a CI check on the move to > "ready to merge". It does not check and I don't think it should attempt to do it. The only task of this script should be to automatically promote the [approval: done] state to [approval: ready to merge] after the grace period has expired, because humans tend to forget that. Anything beyond that, in particular an attempt to judge whether the approval is valid or not, and whether a CI failure can be ignored or not, should be left to humans. Matthias
Re: Github PR label automation
> Does it also check that the CI says that everything is OK? Do we want it to? I assumed that Approval: done was not being applied unless tests past (but looking that's not always the case). Can we assume that something in "approval: ready to merge" but that failed CI won't get merged? Otherwise I could add a CI check on the move to "ready to merge". Mark
Re: Github PR label automation
On Sat, Feb 08, 2020 at 03:56:19PM +, Mark J Cox wrote: > So right now here's what it does: > > Every hour it looks at open PRs that are labelled "approval: done". > If 24 hours has elapsed since that label was assigned and if there > have been no comments made to the PR since the label was assigned then > it is automatically moved to "approval: ready to merge" with a comment > added to trigger notifications. So if you want to stop something > going to "ready to merge" just add any comment to the PR. Does it also check that the CI says that everything is OK?
Re: Github PR label automation
I've seen some projects enjoying https://github.com/apps/stale, though its focus is not so much relabelling things that are ready, but closing things that are not active, which might not be relevant here. On Sun, Feb 9, 2020 at 2:23 AM Mark J Cox wrote: > > Okay, I'll leave things as they are with those issues and just add the > addition of notification on urgent labels. > > As to the earlier question form the thread as why this isn't a github > automation: the actual reason I was writing the script was to get some > on-demand statistics for my own interest and it just turned out this > was something that bugged me that I could use the same framework for > (I did a trivial review and thought how annoying it would be to have > to manually set a reminder myself to do a future action). I was a > pretty trivial (few hours) script, so I have no concern if it is done > differently or not used in the future should a better way present > itself. > > Mark > > On Sun, Feb 9, 2020 at 12:19 AM Matt Caswell wrote: > > > > > > > > On 08/02/2020 15:56, Mark J Cox wrote: > > > I've currently got a cron job running every hour that looks at open PR > > > requests against github openssl repo and does various actions. So if > > > you were wondering why I was altering labels and making comments at > > > 4am, now you know. No doubt we'll use some tool user for this in the > > > future. > > > > > > So right now here's what it does: > > > > > > Every hour it looks at open PRs that are labelled "approval: done". > > > If 24 hours has elapsed since that label was assigned and if there > > > have been no comments made to the PR since the label was assigned then > > > it is automatically moved to "approval: ready to merge" with a comment > > > added to trigger notifications. So if you want to stop something > > > going to "ready to merge" just add any comment to the PR. > > > > > > I'm thinking of using this script also to 1) collect interesting stats > > > and 2) do some other actions. So if there's some automation you'd > > > like to see just add an enhancement issue against the openssl/tools > > > repo. > > > > > > 1 Matt already asked for committer notification trigger for anything > > > labelled Urgent. > > > > > > 2 If there were comments made after "approval: done" then I think we > > > really ought to drop the "approval: done" label as the comments likely > > > invalidated the approval. So I'll likely add that next week (if > > > "approval: done" label and has comments since that label then remove > > > the label and add a comment 'please review if this is really approval: > > > done'. If the approval: done label gets set again then after 24 hours > > > the existing automation will trigger. #10786 is a good example of > > > this. > > > > I'm less sure about this. I think there are probably many examples where > > this is not the case. E.g. a quick review found these recent cases: > > > > https://github.com/openssl/openssl/pull/11003 > > > > https://github.com/openssl/openssl/pull/11015 > > > > https://github.com/openssl/openssl/pull/10992 > > > > https://github.com/openssl/openssl/pull/10991 > > > > https://github.com/openssl/openssl/pull/10990 > > > > https://github.com/openssl/openssl/pull/10989 > > > > I would be more minded to think that if a comment invalidates an > > "approval done" then the committer should explicitly decide to do that. > > > > Matt
Re: Github PR label automation
Okay, I'll leave things as they are with those issues and just add the addition of notification on urgent labels. As to the earlier question form the thread as why this isn't a github automation: the actual reason I was writing the script was to get some on-demand statistics for my own interest and it just turned out this was something that bugged me that I could use the same framework for (I did a trivial review and thought how annoying it would be to have to manually set a reminder myself to do a future action). I was a pretty trivial (few hours) script, so I have no concern if it is done differently or not used in the future should a better way present itself. Mark On Sun, Feb 9, 2020 at 12:19 AM Matt Caswell wrote: > > > > On 08/02/2020 15:56, Mark J Cox wrote: > > I've currently got a cron job running every hour that looks at open PR > > requests against github openssl repo and does various actions. So if > > you were wondering why I was altering labels and making comments at > > 4am, now you know. No doubt we'll use some tool user for this in the > > future. > > > > So right now here's what it does: > > > > Every hour it looks at open PRs that are labelled "approval: done". > > If 24 hours has elapsed since that label was assigned and if there > > have been no comments made to the PR since the label was assigned then > > it is automatically moved to "approval: ready to merge" with a comment > > added to trigger notifications. So if you want to stop something > > going to "ready to merge" just add any comment to the PR. > > > > I'm thinking of using this script also to 1) collect interesting stats > > and 2) do some other actions. So if there's some automation you'd > > like to see just add an enhancement issue against the openssl/tools > > repo. > > > > 1 Matt already asked for committer notification trigger for anything > > labelled Urgent. > > > > 2 If there were comments made after "approval: done" then I think we > > really ought to drop the "approval: done" label as the comments likely > > invalidated the approval. So I'll likely add that next week (if > > "approval: done" label and has comments since that label then remove > > the label and add a comment 'please review if this is really approval: > > done'. If the approval: done label gets set again then after 24 hours > > the existing automation will trigger. #10786 is a good example of > > this. > > I'm less sure about this. I think there are probably many examples where > this is not the case. E.g. a quick review found these recent cases: > > https://github.com/openssl/openssl/pull/11003 > > https://github.com/openssl/openssl/pull/11015 > > https://github.com/openssl/openssl/pull/10992 > > https://github.com/openssl/openssl/pull/10991 > > https://github.com/openssl/openssl/pull/10990 > > https://github.com/openssl/openssl/pull/10989 > > I would be more minded to think that if a comment invalidates an > "approval done" then the committer should explicitly decide to do that. > > Matt
Re: Github PR label automation
On 08/02/2020 15:56, Mark J Cox wrote: > I've currently got a cron job running every hour that looks at open PR > requests against github openssl repo and does various actions. So if > you were wondering why I was altering labels and making comments at > 4am, now you know. No doubt we'll use some tool user for this in the > future. > > So right now here's what it does: > > Every hour it looks at open PRs that are labelled "approval: done". > If 24 hours has elapsed since that label was assigned and if there > have been no comments made to the PR since the label was assigned then > it is automatically moved to "approval: ready to merge" with a comment > added to trigger notifications. So if you want to stop something > going to "ready to merge" just add any comment to the PR. > > I'm thinking of using this script also to 1) collect interesting stats > and 2) do some other actions. So if there's some automation you'd > like to see just add an enhancement issue against the openssl/tools > repo. > > 1 Matt already asked for committer notification trigger for anything > labelled Urgent. > > 2 If there were comments made after "approval: done" then I think we > really ought to drop the "approval: done" label as the comments likely > invalidated the approval. So I'll likely add that next week (if > "approval: done" label and has comments since that label then remove > the label and add a comment 'please review if this is really approval: > done'. If the approval: done label gets set again then after 24 hours > the existing automation will trigger. #10786 is a good example of > this. I'm less sure about this. I think there are probably many examples where this is not the case. E.g. a quick review found these recent cases: https://github.com/openssl/openssl/pull/11003 https://github.com/openssl/openssl/pull/11015 https://github.com/openssl/openssl/pull/10992 https://github.com/openssl/openssl/pull/10991 https://github.com/openssl/openssl/pull/10990 https://github.com/openssl/openssl/pull/10989 I would be more minded to think that if a comment invalidates an "approval done" then the committer should explicitly decide to do that. Matt
Re: Github PR label automation
Please don’t automatically drop the "appoval: done" label after a comment. I feel that is not uncommon for comments to be added that in no way invalidate the approval. I agree with not switching to “ready to merge” if there are comments — manual intervention in this case is required to judge the relevancy. Agreed also over the “urgent” label. Pauli -- Dr Paul Dale | Distinguished Architect | Cryptographic Foundations Phone +61 7 3031 7217 Oracle Australia > On 9 Feb 2020, at 1:56 am, Mark J Cox wrote: > > I've currently got a cron job running every hour that looks at open PR > requests against github openssl repo and does various actions. So if > you were wondering why I was altering labels and making comments at > 4am, now you know. No doubt we'll use some tool user for this in the > future. > > So right now here's what it does: > > Every hour it looks at open PRs that are labelled "approval: done". > If 24 hours has elapsed since that label was assigned and if there > have been no comments made to the PR since the label was assigned then > it is automatically moved to "approval: ready to merge" with a comment > added to trigger notifications. So if you want to stop something > going to "ready to merge" just add any comment to the PR. > > I'm thinking of using this script also to 1) collect interesting stats > and 2) do some other actions. So if there's some automation you'd > like to see just add an enhancement issue against the openssl/tools > repo. > > 1 Matt already asked for committer notification trigger for anything > labelled Urgent. > > 2 If there were comments made after "approval: done" then I think we > really ought to drop the "approval: done" label as the comments likely > invalidated the approval. So I'll likely add that next week (if > "approval: done" label and has comments since that label then remove > the label and add a comment 'please review if this is really approval: > done'. If the approval: done label gets set again then after 24 hours > the existing automation will trigger. #10786 is a good example of > this. > > Mark
RE: Github PR label automation
> -Original Message- > From: openssl-project On Behalf Of Mark > J Cox > Sent: Saturday, February 8, 2020 8:52 PM > To: Dmitry Belyavsky > Cc: openssl-project@openssl.org > Subject: Re: Github PR label automation > > Thanks Dmitry; I hope that the comment triggers notifications to the > creator without mentioning them? (let me know if you get something > changed labels that doesn't) Mark In fact, it was my suggestion *not* to add personal mentions. Since anybody who has posted comments to the pull request (which includes the submitter and all reviewers) will be subscribed to the pull request's thread, I think a general comment which addresses nobody specific (just like our "ping" messages) is less intrusive. Matthias
RE: Github PR label automation
First of all, thank you Mark for implementing the notification daemon. You did a great job and I think it's very useful. Here are some comments and thoughts about your last mail. > No doubt we'll use some tool user for this in the future. Can't you just use an API-token generated for the openssl-machine account, as the issue closing bot does? A propos bot: I thought that GitHub provides official support for this kind of watch-bots we are seeking and that they can be configured or programmed in an event driven fashion. Executing specific actions (like setting labels or posting messages) in response to specific GitHub events (approval added, change requested, timer expired, etc.) would have some advantages compared to an external timer based approach. Did someone of you consider this option and do some investigations in that direction? Please don't misinterpret my question, I think that the current cron job solution is already a great improvement and a big step forward. > 2 If there were comments made after "approval: done" then I think we > really ought to drop the "approval: done" label as the comments likely > invalidated the approval. ... I don't think that an *arbitrary comment* should automatically reset the approval state. Anybody could just post a "nice job" comment or some side note. Resetting the approval state could happen automatically if a *team member* posts a review with *change requests*. But not in any other case (e.g. a change requests by a non-team member or an additional approving review). Also, I am strongly convinced that making the transition from the [approval: * review pending] to the [approval:done] state should remain a *purely manual* state. I don't think it's a good idea to leave this decision to an "artificial intelligence" whatsoever. And just counting whether the number of approvals has reached the required minimum is too simplistic anyway. Just imagine the pull request has reached the required number of reviews, but the submitter or a reviewer is still waiting for another important outstanding review. Do you really want the bot to interfere? What about the existing approvals? They need to be dismissed if 'too much' has changed, but not if the author pushed a trivial fixup addressing an approving review. Do you really want to leave this decision to a bot? This approach will just not work. It is really almost no extra effort if we demand that the second reviewer sets the [approval: done] label manually, unless he sees that there are still unresolved discussions in progress. Only the grace period handling should be automated IMHO. Regards, Matthias
Re: Github PR label automation
Thanks Dmitry; I hope that the comment triggers notifications to the creator without mentioning them? (let me know if you get something changed labels that doesn't) Mark On Sat, Feb 8, 2020 at 4:57 PM Dmitry Belyavsky wrote: > > Dear Mark, > > Thank you for a nice job! > > As the reviewers are expected to commit the PRs, could you also add the > reviewers' names as a part of the notification? > > On Sat, Feb 8, 2020 at 6:56 PM Mark J Cox wrote: >> >> I've currently got a cron job running every hour that looks at open PR >> requests against github openssl repo and does various actions. So if >> you were wondering why I was altering labels and making comments at >> 4am, now you know. No doubt we'll use some tool user for this in the >> future. >> >> So right now here's what it does: >> >> Every hour it looks at open PRs that are labelled "approval: done". >> If 24 hours has elapsed since that label was assigned and if there >> have been no comments made to the PR since the label was assigned then >> it is automatically moved to "approval: ready to merge" with a comment >> added to trigger notifications. So if you want to stop something >> going to "ready to merge" just add any comment to the PR. >> >> I'm thinking of using this script also to 1) collect interesting stats >> and 2) do some other actions. So if there's some automation you'd >> like to see just add an enhancement issue against the openssl/tools >> repo. >> >> 1 Matt already asked for committer notification trigger for anything >> labelled Urgent. >> >> 2 If there were comments made after "approval: done" then I think we >> really ought to drop the "approval: done" label as the comments likely >> invalidated the approval. So I'll likely add that next week (if >> "approval: done" label and has comments since that label then remove >> the label and add a comment 'please review if this is really approval: >> done'. If the approval: done label gets set again then after 24 hours >> the existing automation will trigger. #10786 is a good example of >> this. >> >> Mark > > > > -- > SY, Dmitry Belyavsky
Re: Github PR label automation
Dear Mark, Thank you for a nice job! As the reviewers are expected to commit the PRs, could you also add the reviewers' names as a part of the notification? On Sat, Feb 8, 2020 at 6:56 PM Mark J Cox wrote: > I've currently got a cron job running every hour that looks at open PR > requests against github openssl repo and does various actions. So if > you were wondering why I was altering labels and making comments at > 4am, now you know. No doubt we'll use some tool user for this in the > future. > > So right now here's what it does: > > Every hour it looks at open PRs that are labelled "approval: done". > If 24 hours has elapsed since that label was assigned and if there > have been no comments made to the PR since the label was assigned then > it is automatically moved to "approval: ready to merge" with a comment > added to trigger notifications. So if you want to stop something > going to "ready to merge" just add any comment to the PR. > > I'm thinking of using this script also to 1) collect interesting stats > and 2) do some other actions. So if there's some automation you'd > like to see just add an enhancement issue against the openssl/tools > repo. > > 1 Matt already asked for committer notification trigger for anything > labelled Urgent. > > 2 If there were comments made after "approval: done" then I think we > really ought to drop the "approval: done" label as the comments likely > invalidated the approval. So I'll likely add that next week (if > "approval: done" label and has comments since that label then remove > the label and add a comment 'please review if this is really approval: > done'. If the approval: done label gets set again then after 24 hours > the existing automation will trigger. #10786 is a good example of > this. > > Mark > -- SY, Dmitry Belyavsky
Github PR label automation
I've currently got a cron job running every hour that looks at open PR requests against github openssl repo and does various actions. So if you were wondering why I was altering labels and making comments at 4am, now you know. No doubt we'll use some tool user for this in the future. So right now here's what it does: Every hour it looks at open PRs that are labelled "approval: done". If 24 hours has elapsed since that label was assigned and if there have been no comments made to the PR since the label was assigned then it is automatically moved to "approval: ready to merge" with a comment added to trigger notifications. So if you want to stop something going to "ready to merge" just add any comment to the PR. I'm thinking of using this script also to 1) collect interesting stats and 2) do some other actions. So if there's some automation you'd like to see just add an enhancement issue against the openssl/tools repo. 1 Matt already asked for committer notification trigger for anything labelled Urgent. 2 If there were comments made after "approval: done" then I think we really ought to drop the "approval: done" label as the comments likely invalidated the approval. So I'll likely add that next week (if "approval: done" label and has comments since that label then remove the label and add a comment 'please review if this is really approval: done'. If the approval: done label gets set again then after 24 hours the existing automation will trigger. #10786 is a good example of this. Mark
