Re: Github PR label automation

2020-02-08 Thread Dr Paul Dale
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

2020-02-08 Thread Dr. Matthias St. Pierre

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

2020-02-08 Thread Dr. Matthias St. Pierre
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

2020-02-08 Thread Mark J Cox
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

2020-02-08 Thread Dmitry Belyavsky
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

2020-02-08 Thread Mark J Cox
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