Re: Github PR label automation

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

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

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

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

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

2020-02-11 Thread Kurt Roeckx
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

2020-02-10 Thread Sam Roberts
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

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

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