Re: Skara - bot sending can-be-integrated message prematurely?

[Nir Lisker]

I forgot to mention that I filed these:

https://bugs.openjdk.java.net/browse/SKARA-218
https://bugs.openjdk.java.net/browse/SKARA-217

They weren't triaged, so maybe the Skara team needs to be notified.

On Thu, Dec 19, 2019 at 6:24 PM Kevin Rushforth 
wrote:

> FYI, for anyone interested, the Skara team submitted the following PR to
> modify the "ready for integration" message:
>
> https://github.com/openjdk/skara/pull/343
>
> We can file a follow-up RFE to have them consider adding "/reviewers" and
> "/csr" commands.
>
> -- Kevin
>
>
> On 12/18/2019 7:17 PM, Phil Race wrote:
>
> It would have to allow anyone who has reviewer status to add that comment.
> Not just the author since if they knew we would have less need for it.
>
> -Phil.
>
> On Dec 18, 2019, at 11:31 AM, Kevin Rushforth 
> wrote:
>
> That's an interesting idea. It would, of course, need to disallow reducing
> the number below the minimum specified in .jcheck/conf (e.g., we wouldn't
> allow "/reviewers 0").
>
> -- Kevin
>
>
> On 12/18/2019 10:36 AM, Nir Lisker wrote:
>
> The client libraries in the OpenJDK do as a default rule, excusing simple
>> fixes.
>>
>
> Then maybe it would be helpful to have a "/reviewers n" command that will
> tell the bot how many reviewers are needed. It's less convoluted than the
> CSR tracking and basically replaces the comment a reviewer would make
> anyway to inform the PR author how many reviewers they should wait for.
> Extending the bot to look for n reviewers instead of the current 1 should
> not be far fetched.
>
>
>
>>
>>
> On Wed, Dec 18, 2019 at 4:02 AM Philip Race 
> wrote:
>
>>
>>
>> On 12/16/19, 4:14 PM, Nir Lisker wrote:
>> > Do other projects also have multi-reviewers requirements?
>>
>> The client libraries in the OpenJDK do as a default rule, excusing
>> simple fixes.
>>
>> >
>> > I would think that at least for a CSR, which is OpenJDK-wide, a request
>> > could be put in with the Skara to track it. A Reviewer (or Committer?)
>> > could invoke a /csr command which will require the PR author to provide
>> a
>> > link to the CSR, and check that it was approved as part of the patch
>> > approval process.
>>
>> I think that if there is a CSR sub-task in JBS skara can key off whether
>> that is approved.
>> This does mean skara needs to probe JBS but SFAIK its doing that a
>> hundred times
>> a minute anyway.
>>
>> -phil.
>>
>> >
>> > Not sure how complicated it would be to implement.
>> >
>> > - Nir
>> >
>> > On Mon, Dec 16, 2019 at 5:39 PM Kevin Rushforth<
>> kevin.rushfo...@oracle.com>
>> > wrote:
>> >
>> >> That's a good question about whether we can ask for a slight rewording
>> >> of the Skara bot message to indicate that there might be other things
>> to
>> >> check before "/integrate". I'll raise that question with the Skara
>> team.
>> >>
>> >> As an aside, I noticed the other day that you typed "/integrate" after
>> a
>> >> single review, but in that case, there was no clear indication from
>> Ajit
>> >> (the first reviewer and the sponsor) that a second review was required,
>> >> so I didn't comment on it.
>> >>
>> >> -- Kevin
>> >>
>> >>
>> >> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>> >>> Kevin,
>> >>>
>> >>> thanks for the clarification :) My bad that I didn't re-read the
>> >>> contrib.md. But then, who does? The lazy like myself do it
>> >>> occasionally only (down to once and then forget about it)
>> >>>
>> >>> Maybe the bot message can be improved? With some indication that its
>> >>> (the bot's) knowledge about review requirements is limited, so would
>> >>> require a careful check by the contributor before actually post the
>> >>> /integrate comment? Actually, I think I goofed the other day, was
>> >>> safed only by Ajit who waited for the 2nd review before his /sponsor.
>> >>>
>> >>> -- Jeanette
>> >>>
>> >>> Zitat von Kevin Rushforth:
>> >>>
>>  I added a comment to the two PRs in question, but it bears discussion
>>  here.
>> 
>>  The Skara bot can't know whether all criteria have been met. It
>>  can't, for example, know whether there are outstanding comments from
>>  some reviewers that need to be addressed. Nor does it know which PRs
>>  need two reviewers (or sometimes a third if there is a specific
>>  person we would like to review it), which ones need a CSR, etc.
>> 
>>  So having it state authoritatively that the PR is ready to integrate
>>  is a bit misleading. This is documented in the Code Review section of
>>  the CONTRIBUTING [1] doc:
>> 
>> > NOTE: while the Skara tooling will indicate that the PR is ready to
>> > integrate once the first reviewer with a "Reviewer" role in the
>> > project has approved it, this may or may not be sufficient depending
>> > on the type of fix. For example, you must wait for a second approval
>> > for enhancements or high-impact bug fixes.
>>  If anyone can think of a way to improve this and make it more clear,
>>  that would be helpful.
>> 

Re: Skara - bot sending can-be-integrated message prematurely?

[Kevin Rushforth]

FYI, for anyone interested, the Skara team submitted the following PR to 
modify the "ready for integration" message:


https://github.com/openjdk/skara/pull/343

We can file a follow-up RFE to have them consider adding "/reviewers" 
and "/csr" commands.


-- Kevin


On 12/18/2019 7:17 PM, Phil Race wrote:
It would have to allow anyone who has reviewer status to add that 
comment.

Not just the author since if they knew we would have less need for it.

-Phil.

On Dec 18, 2019, at 11:31 AM, Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


That's an interesting idea. It would, of course, need to disallow 
reducing the number below the minimum specified in .jcheck/conf 
(e.g., we wouldn't allow "/reviewers 0").


-- Kevin


On 12/18/2019 10:36 AM, Nir Lisker wrote:


The client libraries in the OpenJDK do as a default rule,
excusing simple fixes.


Then maybe it would be helpful to have a "/reviewers n" command that 
will tell the bot how many reviewers are needed. It's less 
convoluted than the CSR tracking and basically replaces the comment 
a reviewer would make anyway to inform the PR author how many 
reviewers they should wait for. Extending the bot to look for n 
reviewers instead of the current 1 should not be far fetched.




On Wed, Dec 18, 2019 at 4:02 AM Philip Race > wrote:




On 12/16/19, 4:14 PM, Nir Lisker wrote:
> Do other projects also have multi-reviewers requirements?

The client libraries in the OpenJDK do as a default rule, excusing
simple fixes.

>
> I would think that at least for a CSR, which is OpenJDK-wide,
a request
> could be put in with the Skara to track it. A Reviewer (or
Committer?)
> could invoke a /csr command which will require the PR author
to provide a
> link to the CSR, and check that it was approved as part of the
patch
> approval process.

I think that if there is a CSR sub-task in JBS skara can key off
whether
that is approved.
This does mean skara needs to probe JBS but SFAIK its doing that a
hundred times
a minute anyway.

-phil.

>
> Not sure how complicated it would be to implement.
>
> - Nir
>
> On Mon, Dec 16, 2019 at 5:39 PM Kevin
Rushforthmailto:kevin.rushfo...@oracle.com>>
> wrote:
>
>> That's a good question about whether we can ask for a slight
rewording
>> of the Skara bot message to indicate that there might be
other things to
>> check before "/integrate". I'll raise that question with the
Skara team.
>>
>> As an aside, I noticed the other day that you typed
"/integrate" after a
>> single review, but in that case, there was no clear
indication from Ajit
>> (the first reviewer and the sponsor) that a second review was
required,
>> so I didn't comment on it.
>>
>> -- Kevin
>>
>>
>> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>>> Kevin,
>>>
>>> thanks for the clarification :) My bad that I didn't re-read the
>>> contrib.md. But then, who does? The lazy like myself do it
>>> occasionally only (down to once and then forget about it)
>>>
>>> Maybe the bot message can be improved? With some indication
that its
>>> (the bot's) knowledge about review requirements is limited,
so would
>>> require a careful check by the contributor before actually
post the
>>> /integrate comment? Actually, I think I goofed the other
day, was
>>> safed only by Ajit who waited for the 2nd review before his
/sponsor.
>>>
>>> -- Jeanette
>>>
>>> Zitat von Kevin Rushforthmailto:kevin.rushfo...@oracle.com>>:
>>>
 I added a comment to the two PRs in question, but it bears
discussion
 here.

 The Skara bot can't know whether all criteria have been met. It
 can't, for example, know whether there are outstanding
comments from
 some reviewers that need to be addressed. Nor does it know
which PRs
 need two reviewers (or sometimes a third if there is a specific
 person we would like to review it), which ones need a CSR, etc.

 So having it state authoritatively that the PR is ready to
integrate
 is a bit misleading. This is documented in the Code Review
section of
 the CONTRIBUTING [1] doc:

> NOTE: while the Skara tooling will indicate that the PR is
ready to
> integrate once the first reviewer with a "Reviewer" role
in the
> project has approved it, this may or may not be sufficient
depending
> on the type of fix. For example, you must wait for a
second approval
> for enhancements or high-impact bug fixes.
 If anyone can think of a way to improve this and make it
more clear,
 that would be helpful.

 -- Kevin

 [1] 

Re: Skara - bot sending can-be-integrated message prematurely?

[Phil Race]

It would have to allow anyone who has reviewer status to add that comment.
Not just the author since if they knew we would have less need for it.

-Phil.

> On Dec 18, 2019, at 11:31 AM, Kevin Rushforth  
> wrote:
> 
> That's an interesting idea. It would, of course, need to disallow reducing 
> the number below the minimum specified in .jcheck/conf (e.g., we wouldn't 
> allow "/reviewers 0").
> 
> -- Kevin
> 
> 
>> On 12/18/2019 10:36 AM, Nir Lisker wrote:
>>> The client libraries in the OpenJDK do as a default rule, excusing simple 
>>> fixes.
>> 
>> Then maybe it would be helpful to have a "/reviewers n" command that will 
>> tell the bot how many reviewers are needed. It's less convoluted than the 
>> CSR tracking and basically replaces the comment a reviewer would make anyway 
>> to inform the PR author how many reviewers they should wait for. Extending 
>> the bot to look for n reviewers instead of the current 1 should not be far 
>> fetched.
>> 
>>  
>>> 
>> 
>>> On Wed, Dec 18, 2019 at 4:02 AM Philip Race  wrote:
>>> 
>>> 
>>> On 12/16/19, 4:14 PM, Nir Lisker wrote:
>>> > Do other projects also have multi-reviewers requirements?
>>> 
>>> The client libraries in the OpenJDK do as a default rule, excusing 
>>> simple fixes.
>>> 
>>> >
>>> > I would think that at least for a CSR, which is OpenJDK-wide, a request
>>> > could be put in with the Skara to track it. A Reviewer (or Committer?)
>>> > could invoke a /csr command which will require the PR author to provide a
>>> > link to the CSR, and check that it was approved as part of the patch
>>> > approval process.
>>> 
>>> I think that if there is a CSR sub-task in JBS skara can key off whether 
>>> that is approved.
>>> This does mean skara needs to probe JBS but SFAIK its doing that a 
>>> hundred times
>>> a minute anyway.
>>> 
>>> -phil.
>>> 
>>> >
>>> > Not sure how complicated it would be to implement.
>>> >
>>> > - Nir
>>> >
>>> > On Mon, Dec 16, 2019 at 5:39 PM Kevin 
>>> > Rushforth
>>> > wrote:
>>> >
>>> >> That's a good question about whether we can ask for a slight rewording
>>> >> of the Skara bot message to indicate that there might be other things to
>>> >> check before "/integrate". I'll raise that question with the Skara team.
>>> >>
>>> >> As an aside, I noticed the other day that you typed "/integrate" after a
>>> >> single review, but in that case, there was no clear indication from Ajit
>>> >> (the first reviewer and the sponsor) that a second review was required,
>>> >> so I didn't comment on it.
>>> >>
>>> >> -- Kevin
>>> >>
>>> >>
>>> >> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>>> >>> Kevin,
>>> >>>
>>> >>> thanks for the clarification :) My bad that I didn't re-read the
>>> >>> contrib.md. But then, who does? The lazy like myself do it
>>> >>> occasionally only (down to once and then forget about it)
>>> >>>
>>> >>> Maybe the bot message can be improved? With some indication that its
>>> >>> (the bot's) knowledge about review requirements is limited, so would
>>> >>> require a careful check by the contributor before actually post the
>>> >>> /integrate comment? Actually, I think I goofed the other day, was
>>> >>> safed only by Ajit who waited for the 2nd review before his /sponsor.
>>> >>>
>>> >>> -- Jeanette
>>> >>>
>>> >>> Zitat von Kevin Rushforth:
>>> >>>
>>>  I added a comment to the two PRs in question, but it bears discussion
>>>  here.
>>> 
>>>  The Skara bot can't know whether all criteria have been met. It
>>>  can't, for example, know whether there are outstanding comments from
>>>  some reviewers that need to be addressed. Nor does it know which PRs
>>>  need two reviewers (or sometimes a third if there is a specific
>>>  person we would like to review it), which ones need a CSR, etc.
>>> 
>>>  So having it state authoritatively that the PR is ready to integrate
>>>  is a bit misleading. This is documented in the Code Review section of
>>>  the CONTRIBUTING [1] doc:
>>> 
>>> > NOTE: while the Skara tooling will indicate that the PR is ready to
>>> > integrate once the first reviewer with a "Reviewer" role in the
>>> > project has approved it, this may or may not be sufficient depending
>>> > on the type of fix. For example, you must wait for a second approval
>>> > for enhancements or high-impact bug fixes.
>>>  If anyone can think of a way to improve this and make it more clear,
>>>  that would be helpful.
>>> 
>>>  -- Kevin
>>> 
>>>  [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
>>> 
>>> 
>>>  On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
>>> > Looks like it assumes a pull request as properly reviewed as soon as
>>> > it gets a single approve - independent on how many reviewers are
>>> > required, see f.i.
>>> >
>>> > https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
>>> > https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
>>> 

Re: Skara - bot sending can-be-integrated message prematurely?

[Kevin Rushforth]

That's an interesting idea. It would, of course, need to disallow 
reducing the number below the minimum specified in .jcheck/conf (e.g., 
we wouldn't allow "/reviewers 0").


-- Kevin


On 12/18/2019 10:36 AM, Nir Lisker wrote:


The client libraries in the OpenJDK do as a default rule,
excusing simple fixes.


Then maybe it would be helpful to have a "/reviewers n" command that 
will tell the bot how many reviewers are needed. It's less convoluted 
than the CSR tracking and basically replaces the comment a reviewer 
would make anyway to inform the PR author how many reviewers they 
should wait for. Extending the bot to look for n reviewers instead of 
the current 1 should not be far fetched.




On Wed, Dec 18, 2019 at 4:02 AM Philip Race > wrote:




On 12/16/19, 4:14 PM, Nir Lisker wrote:
> Do other projects also have multi-reviewers requirements?

The client libraries in the OpenJDK do as a default rule, excusing
simple fixes.

>
> I would think that at least for a CSR, which is OpenJDK-wide, a
request
> could be put in with the Skara to track it. A Reviewer (or
Committer?)
> could invoke a /csr command which will require the PR author to
provide a
> link to the CSR, and check that it was approved as part of the patch
> approval process.

I think that if there is a CSR sub-task in JBS skara can key off
whether
that is approved.
This does mean skara needs to probe JBS but SFAIK its doing that a
hundred times
a minute anyway.

-phil.

>
> Not sure how complicated it would be to implement.
>
> - Nir
>
> On Mon, Dec 16, 2019 at 5:39 PM Kevin
Rushforthmailto:kevin.rushfo...@oracle.com>>
> wrote:
>
>> That's a good question about whether we can ask for a slight
rewording
>> of the Skara bot message to indicate that there might be other
things to
>> check before "/integrate". I'll raise that question with the
Skara team.
>>
>> As an aside, I noticed the other day that you typed
"/integrate" after a
>> single review, but in that case, there was no clear indication
from Ajit
>> (the first reviewer and the sponsor) that a second review was
required,
>> so I didn't comment on it.
>>
>> -- Kevin
>>
>>
>> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>>> Kevin,
>>>
>>> thanks for the clarification :) My bad that I didn't re-read the
>>> contrib.md. But then, who does? The lazy like myself do it
>>> occasionally only (down to once and then forget about it)
>>>
>>> Maybe the bot message can be improved? With some indication
that its
>>> (the bot's) knowledge about review requirements is limited, so
would
>>> require a careful check by the contributor before actually
post the
>>> /integrate comment? Actually, I think I goofed the other day, was
>>> safed only by Ajit who waited for the 2nd review before his
/sponsor.
>>>
>>> -- Jeanette
>>>
>>> Zitat von Kevin Rushforthmailto:kevin.rushfo...@oracle.com>>:
>>>
 I added a comment to the two PRs in question, but it bears
discussion
 here.

 The Skara bot can't know whether all criteria have been met. It
 can't, for example, know whether there are outstanding
comments from
 some reviewers that need to be addressed. Nor does it know
which PRs
 need two reviewers (or sometimes a third if there is a specific
 person we would like to review it), which ones need a CSR, etc.

 So having it state authoritatively that the PR is ready to
integrate
 is a bit misleading. This is documented in the Code Review
section of
 the CONTRIBUTING [1] doc:

> NOTE: while the Skara tooling will indicate that the PR is
ready to
> integrate once the first reviewer with a "Reviewer" role in the
> project has approved it, this may or may not be sufficient
depending
> on the type of fix. For example, you must wait for a second
approval
> for enhancements or high-impact bug fixes.
 If anyone can think of a way to improve this and make it more
clear,
 that would be helpful.

 -- Kevin

 [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md


 On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
> Looks like it assumes a pull request as properly reviewed as
soon as
> it gets a single approve - independent on how many reviewers are
> required, see f.i.
>
> https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
> https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
>
> -- Jeanette
>
>>>
>>>
>>

Re: Skara - bot sending can-be-integrated message prematurely?

[Nir Lisker]

>
> The client libraries in the OpenJDK do as a default rule, excusing simple
> fixes.
>

Then maybe it would be helpful to have a "/reviewers n" command that will
tell the bot how many reviewers are needed. It's less convoluted than the
CSR tracking and basically replaces the comment a reviewer would make
anyway to inform the PR author how many reviewers they should wait for.
Extending the bot to look for n reviewers instead of the current 1 should
not be far fetched.



>
>
On Wed, Dec 18, 2019 at 4:02 AM Philip Race  wrote:

>
>
> On 12/16/19, 4:14 PM, Nir Lisker wrote:
> > Do other projects also have multi-reviewers requirements?
>
> The client libraries in the OpenJDK do as a default rule, excusing
> simple fixes.
>
> >
> > I would think that at least for a CSR, which is OpenJDK-wide, a request
> > could be put in with the Skara to track it. A Reviewer (or Committer?)
> > could invoke a /csr command which will require the PR author to provide a
> > link to the CSR, and check that it was approved as part of the patch
> > approval process.
>
> I think that if there is a CSR sub-task in JBS skara can key off whether
> that is approved.
> This does mean skara needs to probe JBS but SFAIK its doing that a
> hundred times
> a minute anyway.
>
> -phil.
>
> >
> > Not sure how complicated it would be to implement.
> >
> > - Nir
> >
> > On Mon, Dec 16, 2019 at 5:39 PM Kevin Rushforth<
> kevin.rushfo...@oracle.com>
> > wrote:
> >
> >> That's a good question about whether we can ask for a slight rewording
> >> of the Skara bot message to indicate that there might be other things to
> >> check before "/integrate". I'll raise that question with the Skara team.
> >>
> >> As an aside, I noticed the other day that you typed "/integrate" after a
> >> single review, but in that case, there was no clear indication from Ajit
> >> (the first reviewer and the sponsor) that a second review was required,
> >> so I didn't comment on it.
> >>
> >> -- Kevin
> >>
> >>
> >> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
> >>> Kevin,
> >>>
> >>> thanks for the clarification :) My bad that I didn't re-read the
> >>> contrib.md. But then, who does? The lazy like myself do it
> >>> occasionally only (down to once and then forget about it)
> >>>
> >>> Maybe the bot message can be improved? With some indication that its
> >>> (the bot's) knowledge about review requirements is limited, so would
> >>> require a careful check by the contributor before actually post the
> >>> /integrate comment? Actually, I think I goofed the other day, was
> >>> safed only by Ajit who waited for the 2nd review before his /sponsor.
> >>>
> >>> -- Jeanette
> >>>
> >>> Zitat von Kevin Rushforth:
> >>>
>  I added a comment to the two PRs in question, but it bears discussion
>  here.
> 
>  The Skara bot can't know whether all criteria have been met. It
>  can't, for example, know whether there are outstanding comments from
>  some reviewers that need to be addressed. Nor does it know which PRs
>  need two reviewers (or sometimes a third if there is a specific
>  person we would like to review it), which ones need a CSR, etc.
> 
>  So having it state authoritatively that the PR is ready to integrate
>  is a bit misleading. This is documented in the Code Review section of
>  the CONTRIBUTING [1] doc:
> 
> > NOTE: while the Skara tooling will indicate that the PR is ready to
> > integrate once the first reviewer with a "Reviewer" role in the
> > project has approved it, this may or may not be sufficient depending
> > on the type of fix. For example, you must wait for a second approval
> > for enhancements or high-impact bug fixes.
>  If anyone can think of a way to improve this and make it more clear,
>  that would be helpful.
> 
>  -- Kevin
> 
>  [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
> 
> 
>  On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
> > Looks like it assumes a pull request as properly reviewed as soon as
> > it gets a single approve - independent on how many reviewers are
> > required, see f.i.
> >
> > https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
> > https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
> >
> > -- Jeanette
> >
> >>>
> >>>
> >>
>

Re: Re: Skara - bot sending can-be-integrated message prematurely?

[Eric Bresie]

So is there some “review complete” or “review closed” or some following state 
available for the bot to key on maybe instead of reacting to one reviewer 
completion?

Eric Bresie
ebre...@gmail.com
> On December 16, 2019 at 9:38:00 AM CST, Kevin Rushforth 
>  wrote:
> That's a good question about whether we can ask for a slight rewording
> of the Skara bot message to indicate that there might be other things to
> check before "/integrate". I'll raise that question with the Skara team.
>
> As an aside, I noticed the other day that you typed "/integrate" after a
> single review, but in that case, there was no clear indication from Ajit
> (the first reviewer and the sponsor) that a second review was required,
> so I didn't comment on it.
>
> -- Kevin
>
>
> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
> >
> > Kevin,
> >
> > thanks for the clarification :) My bad that I didn't re-read the
> > contrib.md. But then, who does? The lazy like myself do it
> > occasionally only (down to once and then forget about it )
> >
> > Maybe the bot message can be improved? With some indication that its
> > (the bot's) knowledge about review requirements is limited, so would
> > require a careful check by the contributor before actually post the
> > /integrate comment? Actually, I think I goofed the other day, was
> > safed only by Ajit who waited for the 2nd review before his /sponsor.
> >
> > -- Jeanette
> >
> > Zitat von Kevin Rushforth :
> >
> > > I added a comment to the two PRs in question, but it bears discussion
> > > here.
> > >
> > > The Skara bot can't know whether all criteria have been met. It
> > > can't, for example, know whether there are outstanding comments from
> > > some reviewers that need to be addressed. Nor does it know which PRs
> > > need two reviewers (or sometimes a third if there is a specific
> > > person we would like to review it), which ones need a CSR, etc.
> > >
> > > So having it state authoritatively that the PR is ready to integrate
> > > is a bit misleading. This is documented in the Code Review section of
> > > the CONTRIBUTING [1] doc:
> > >
> > > > NOTE: while the Skara tooling will indicate that the PR is ready to
> > > > integrate once the first reviewer with a "Reviewer" role in the
> > > > project has approved it, this may or may not be sufficient depending
> > > > on the type of fix. For example, you must wait for a second approval
> > > > for enhancements or high-impact bug fixes.
> > >
> > > If anyone can think of a way to improve this and make it more clear,
> > > that would be helpful.
> > >
> > > -- Kevin
> > >
> > > [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
> > >
> > >
> > > On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
> > > >
> > > > Looks like it assumes a pull request as properly reviewed as soon as
> > > > it gets a single approve - independent on how many reviewers are
> > > > required, see f.i.
> > > >
> > > > https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
> > > > https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
> > > >
> > > > -- Jeanette
> > > >
> >
> >
> >
>

Re: Skara - bot sending can-be-integrated message prematurely?

[Philip Race]

On 12/16/19, 4:14 PM, Nir Lisker wrote:

Do other projects also have multi-reviewers requirements?


The client libraries in the OpenJDK do as a default rule, excusing 
simple fixes.




I would think that at least for a CSR, which is OpenJDK-wide, a request
could be put in with the Skara to track it. A Reviewer (or Committer?)
could invoke a /csr command which will require the PR author to provide a
link to the CSR, and check that it was approved as part of the patch
approval process.


I think that if there is a CSR sub-task in JBS skara can key off whether 
that is approved.
This does mean skara needs to probe JBS but SFAIK its doing that a 
hundred times

a minute anyway.

-phil.



Not sure how complicated it would be to implement.

- Nir

On Mon, Dec 16, 2019 at 5:39 PM Kevin Rushforth
wrote:


That's a good question about whether we can ask for a slight rewording
of the Skara bot message to indicate that there might be other things to
check before "/integrate". I'll raise that question with the Skara team.

As an aside, I noticed the other day that you typed "/integrate" after a
single review, but in that case, there was no clear indication from Ajit
(the first reviewer and the sponsor) that a second review was required,
so I didn't comment on it.

-- Kevin


On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:

Kevin,

thanks for the clarification :) My bad that I didn't re-read the
contrib.md. But then, who does? The lazy like myself do it
occasionally only (down to once and then forget about it)

Maybe the bot message can be improved? With some indication that its
(the bot's) knowledge about review requirements is limited, so would
require a careful check by the contributor before actually post the
/integrate comment? Actually, I think I goofed the other day, was
safed only by Ajit who waited for the 2nd review before his /sponsor.

-- Jeanette

Zitat von Kevin Rushforth:


I added a comment to the two PRs in question, but it bears discussion
here.

The Skara bot can't know whether all criteria have been met. It
can't, for example, know whether there are outstanding comments from
some reviewers that need to be addressed. Nor does it know which PRs
need two reviewers (or sometimes a third if there is a specific
person we would like to review it), which ones need a CSR, etc.

So having it state authoritatively that the PR is ready to integrate
is a bit misleading. This is documented in the Code Review section of
the CONTRIBUTING [1] doc:


NOTE: while the Skara tooling will indicate that the PR is ready to
integrate once the first reviewer with a "Reviewer" role in the
project has approved it, this may or may not be sufficient depending
on the type of fix. For example, you must wait for a second approval
for enhancements or high-impact bug fixes.

If anyone can think of a way to improve this and make it more clear,
that would be helpful.

-- Kevin

[1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md


On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:

Looks like it assumes a pull request as properly reviewed as soon as
it gets a single approve - independent on how many reviewers are
required, see f.i.

https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: Skara - bot sending can-be-integrated message prematurely?

[Kevin Rushforth]

I like that suggestion. I'll run it by the Skara team.

-- Kevin


On 12/17/2019 4:50 PM, Scott Palmer wrote:

Perhaps the language in the message should be tweaked?
Maybe use, “This change can be integrated if there are no further review 
requirements.“

Scott


On Dec 17, 2019, at 4:57 PM, Kevin Rushforth  wrote:

Yes, other projects have multiple review requirements. And similar to JavaFX, the answer 
to how many reviewers are needed is "it depends"; sometimes one is enough for 
certain sub-projects or simple fixes; other sub-projects want two reviewers; sometimes 
you want a specific expert to review a tricky change in one area being touched by a fix 
in addition to the two primary reviewers.

I think if Skara tries to get into the business of covering all cases, we will 
be adding a lot of complexity and still not be satisfied. The current approach, 
which I think is the right one for now, is to for the tool to enforce the 
minimum requirement for any fix to be integrated. Beyond that it is up to the 
Committer and Reviewer(s) to make sure that the all criteria for integrating a 
fix have been met.

-- Kevin



On 12/16/2019 4:14 PM, Nir Lisker wrote:
Do other projects also have multi-reviewers requirements?

I would think that at least for a CSR, which is OpenJDK-wide, a request could 
be put in with the Skara to track it. A Reviewer (or Committer?) could invoke a 
/csr command which will require the PR author to provide a link to the CSR, and 
check that it was approved as part of the patch approval process.

Not sure how complicated it would be to implement.

- Nir

On Mon, Dec 16, 2019 at 5:39 PM Kevin Rushforth mailto:kevin.rushfo...@oracle.com>> wrote:

That's a good question about whether we can ask for a slight
rewording
of the Skara bot message to indicate that there might be other
things to
check before "/integrate". I'll raise that question with the Skara
team.

As an aside, I noticed the other day that you typed "/integrate"
after a
single review, but in that case, there was no clear indication
from Ajit
(the first reviewer and the sponsor) that a second review was
required,
so I didn't comment on it.

-- Kevin


On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>
> Kevin,
>
> thanks for the clarification :) My bad that I didn't re-read the
> contrib.md. But then, who does? The lazy like myself do it
> occasionally only (down to once and then forget about it )
>
> Maybe the bot message can be improved? With some indication that
its
> (the bot's) knowledge about review requirements is limited, so
would
> require a careful check by the contributor before actually post the
> /integrate comment? Actually, I think I goofed the other day, was
> safed only by Ajit who waited for the 2nd review before his
/sponsor.
>
> -- Jeanette
>
> Zitat von Kevin Rushforth mailto:kevin.rushfo...@oracle.com>>:
>
>> I added a comment to the two PRs in question, but it bears
discussion
>> here.
>>
>> The Skara bot can't know whether all criteria have been met. It
>> can't, for example, know whether there are outstanding comments
from
>> some reviewers that need to be addressed. Nor does it know
which PRs
>> need two reviewers (or sometimes a third if there is a specific
>> person we would like to review it), which ones need a CSR, etc.
>>
>> So having it state authoritatively that the PR is ready to
integrate
>> is a bit misleading. This is documented in the Code Review
section of
>> the CONTRIBUTING [1] doc:
>>
>>> NOTE: while the Skara tooling will indicate that the PR is
ready to
>>> integrate once the first reviewer with a "Reviewer" role in the
>>> project has approved it, this may or may not be sufficient
depending
>>> on the type of fix. For example, you must wait for a second
approval
>>> for enhancements or high-impact bug fixes.
>>
>> If anyone can think of a way to improve this and make it more
clear,
>> that would be helpful.
>>
>> -- Kevin
>>
>> [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
>>
>>
>> On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
>>>
>>> Looks like it assumes a pull request as properly reviewed as
soon as
>>> it gets a single approve - independent on how many reviewers are
>>> required, see f.i.
>>>
>>> https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
>>> https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
>>>
>>> -- Jeanette
>>>
>
>
>

Re: Skara - bot sending can-be-integrated message prematurely?

[Scott Palmer]

Perhaps the language in the message should be tweaked?
Maybe use, “This change can be integrated if there are no further review 
requirements.“

Scott

> On Dec 17, 2019, at 4:57 PM, Kevin Rushforth  
> wrote:
> 
> Yes, other projects have multiple review requirements. And similar to 
> JavaFX, the answer to how many reviewers are needed is "it depends"; 
> sometimes one is enough for certain sub-projects or simple fixes; other 
> sub-projects want two reviewers; sometimes you want a specific expert to 
> review a tricky change in one area being touched by a fix in addition to the 
> two primary reviewers.
> 
> I think if Skara tries to get into the business of covering all cases, we 
> will be adding a lot of complexity and still not be satisfied. The current 
> approach, which I think is the right one for now, is to for the tool to 
> enforce the minimum requirement for any fix to be integrated. Beyond that it 
> is up to the Committer and Reviewer(s) to make sure that the all criteria for 
> integrating a fix have been met.
> 
> -- Kevin
> 
> 
>> On 12/16/2019 4:14 PM, Nir Lisker wrote:
>> Do other projects also have multi-reviewers requirements?
>> 
>> I would think that at least for a CSR, which is OpenJDK-wide, a request 
>> could be put in with the Skara to track it. A Reviewer (or Committer?) could 
>> invoke a /csr command which will require the PR author to provide a link to 
>> the CSR, and check that it was approved as part of the patch approval 
>> process.
>> 
>> Not sure how complicated it would be to implement.
>> 
>> - Nir
>> 
>> On Mon, Dec 16, 2019 at 5:39 PM Kevin Rushforth > > wrote:
>> 
>>That's a good question about whether we can ask for a slight
>>rewording
>>of the Skara bot message to indicate that there might be other
>>things to
>>check before "/integrate". I'll raise that question with the Skara
>>team.
>> 
>>As an aside, I noticed the other day that you typed "/integrate"
>>after a
>>single review, but in that case, there was no clear indication
>>from Ajit
>>(the first reviewer and the sponsor) that a second review was
>>required,
>>so I didn't comment on it.
>> 
>>-- Kevin
>> 
>> 
>>On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>>>
>>> Kevin,
>>>
>>> thanks for the clarification :) My bad that I didn't re-read the
>>> contrib.md. But then, who does? The lazy like myself do it
>>> occasionally only (down to once and then forget about it )
>>>
>>> Maybe the bot message can be improved? With some indication that
>>its
>>> (the bot's) knowledge about review requirements is limited, so
>>would
>>> require a careful check by the contributor before actually post the
>>> /integrate comment? Actually, I think I goofed the other day, was
>>> safed only by Ajit who waited for the 2nd review before his
>>/sponsor.
>>>
>>> -- Jeanette
>>>
>>> Zitat von Kevin Rushforth >>:
>>>
>>>> I added a comment to the two PRs in question, but it bears
>>discussion
>>>> here.
>>>>
>>>> The Skara bot can't know whether all criteria have been met. It
>>>> can't, for example, know whether there are outstanding comments
>>from
>>>> some reviewers that need to be addressed. Nor does it know
>>which PRs
>>>> need two reviewers (or sometimes a third if there is a specific
>>>> person we would like to review it), which ones need a CSR, etc.
>>>>
>>>> So having it state authoritatively that the PR is ready to
>>integrate
>>>> is a bit misleading. This is documented in the Code Review
>>section of
>>>> the CONTRIBUTING [1] doc:
>>>>
>>>>> NOTE: while the Skara tooling will indicate that the PR is
>>ready to
>>>>> integrate once the first reviewer with a "Reviewer" role in the
>>>>> project has approved it, this may or may not be sufficient
>>depending
>>>>> on the type of fix. For example, you must wait for a second
>>approval
>>>>> for enhancements or high-impact bug fixes.
>>>>
>>>> If anyone can think of a way to improve this and make it more
>>clear,
>>>> that would be helpful.
>>>>
>>>> -- Kevin
>>>>
>>>> [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
>>>>
>>>>
>>>> On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
>>>>>
>>>>> Looks like it assumes a pull request as properly reviewed as
>>soon as
>>>>> it gets a single approve - independent on how many reviewers are
>>>>> required, see f.i.
>>>>>
>>>>> https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
>>>>> https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
>>>>>
>>>>> -- Jeanette
>>>>>
>>>
>>>
>>>
>> 
> 

Re: Skara - bot sending can-be-integrated message prematurely?

[Kevin Rushforth]

Yes, other projects have multiple review requirements. And similar to 
JavaFX, the answer to how many reviewers are needed is "it depends"; 
sometimes one is enough for certain sub-projects or simple fixes; other 
sub-projects want two reviewers; sometimes you want a specific expert to 
review a tricky change in one area being touched by a fix in addition to 
the two primary reviewers.


I think if Skara tries to get into the business of covering all cases, 
we will be adding a lot of complexity and still not be satisfied. The 
current approach, which I think is the right one for now, is to for the 
tool to enforce the minimum requirement for any fix to be integrated. 
Beyond that it is up to the Committer and Reviewer(s) to make sure that 
the all criteria for integrating a fix have been met.


-- Kevin


On 12/16/2019 4:14 PM, Nir Lisker wrote:

Do other projects also have multi-reviewers requirements?

I would think that at least for a CSR, which is OpenJDK-wide, a 
request could be put in with the Skara to track it. A Reviewer (or 
Committer?) could invoke a /csr command which will require the PR 
author to provide a link to the CSR, and check that it was approved as 
part of the patch approval process.


Not sure how complicated it would be to implement.

- Nir

On Mon, Dec 16, 2019 at 5:39 PM Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


That's a good question about whether we can ask for a slight
rewording
of the Skara bot message to indicate that there might be other
things to
check before "/integrate". I'll raise that question with the Skara
team.

As an aside, I noticed the other day that you typed "/integrate"
after a
single review, but in that case, there was no clear indication
from Ajit
(the first reviewer and the sponsor) that a second review was
required,
so I didn't comment on it.

-- Kevin


On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>
> Kevin,
>
> thanks for the clarification :) My bad that I didn't re-read the
> contrib.md. But then, who does? The lazy like myself do it
> occasionally only (down to once and then forget about it )
>
> Maybe the bot message can be improved? With some indication that
its
> (the bot's) knowledge about review requirements is limited, so
would
> require a careful check by the contributor before actually post the
> /integrate comment? Actually, I think I goofed the other day, was
> safed only by Ajit who waited for the 2nd review before his
/sponsor.
>
> -- Jeanette
>
> Zitat von Kevin Rushforth mailto:kevin.rushfo...@oracle.com>>:
>
>> I added a comment to the two PRs in question, but it bears
discussion
>> here.
>>
>> The Skara bot can't know whether all criteria have been met. It
>> can't, for example, know whether there are outstanding comments
from
>> some reviewers that need to be addressed. Nor does it know
which PRs
>> need two reviewers (or sometimes a third if there is a specific
>> person we would like to review it), which ones need a CSR, etc.
>>
>> So having it state authoritatively that the PR is ready to
integrate
>> is a bit misleading. This is documented in the Code Review
section of
>> the CONTRIBUTING [1] doc:
>>
>>> NOTE: while the Skara tooling will indicate that the PR is
ready to
>>> integrate once the first reviewer with a "Reviewer" role in the
>>> project has approved it, this may or may not be sufficient
depending
>>> on the type of fix. For example, you must wait for a second
approval
>>> for enhancements or high-impact bug fixes.
>>
>> If anyone can think of a way to improve this and make it more
clear,
>> that would be helpful.
>>
>> -- Kevin
>>
>> [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
>>
>>
>> On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
>>>
>>> Looks like it assumes a pull request as properly reviewed as
soon as
>>> it gets a single approve - independent on how many reviewers are
>>> required, see f.i.
>>>
>>> https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
>>> https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
>>>
>>> -- Jeanette
>>>
>
>
>

Re: Skara - bot sending can-be-integrated message prematurely?

[Nir Lisker]

Do other projects also have multi-reviewers requirements?

I would think that at least for a CSR, which is OpenJDK-wide, a request
could be put in with the Skara to track it. A Reviewer (or Committer?)
could invoke a /csr command which will require the PR author to provide a
link to the CSR, and check that it was approved as part of the patch
approval process.

Not sure how complicated it would be to implement.

- Nir

On Mon, Dec 16, 2019 at 5:39 PM Kevin Rushforth 
wrote:

> That's a good question about whether we can ask for a slight rewording
> of the Skara bot message to indicate that there might be other things to
> check before "/integrate". I'll raise that question with the Skara team.
>
> As an aside, I noticed the other day that you typed "/integrate" after a
> single review, but in that case, there was no clear indication from Ajit
> (the first reviewer and the sponsor) that a second review was required,
> so I didn't comment on it.
>
> -- Kevin
>
>
> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
> >
> > Kevin,
> >
> > thanks for the clarification :) My bad that I didn't re-read the
> > contrib.md. But then, who does? The lazy like myself do it
> > occasionally only (down to once and then forget about it )
> >
> > Maybe the bot message can be improved? With some indication that its
> > (the bot's) knowledge about review requirements is limited, so would
> > require a careful check by the contributor before actually post the
> > /integrate comment? Actually, I think I goofed the other day, was
> > safed only by Ajit who waited for the 2nd review before his /sponsor.
> >
> > -- Jeanette
> >
> > Zitat von Kevin Rushforth :
> >
> >> I added a comment to the two PRs in question, but it bears discussion
> >> here.
> >>
> >> The Skara bot can't know whether all criteria have been met. It
> >> can't, for example, know whether there are outstanding comments from
> >> some reviewers that need to be addressed. Nor does it know which PRs
> >> need two reviewers (or sometimes a third if there is a specific
> >> person we would like to review it), which ones need a CSR, etc.
> >>
> >> So having it state authoritatively that the PR is ready to integrate
> >> is a bit misleading. This is documented in the Code Review section of
> >> the CONTRIBUTING [1] doc:
> >>
> >>> NOTE: while the Skara tooling will indicate that the PR is ready to
> >>> integrate once the first reviewer with a "Reviewer" role in the
> >>> project has approved it, this may or may not be sufficient depending
> >>> on the type of fix. For example, you must wait for a second approval
> >>> for enhancements or high-impact bug fixes.
> >>
> >> If anyone can think of a way to improve this and make it more clear,
> >> that would be helpful.
> >>
> >> -- Kevin
> >>
> >> [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
> >>
> >>
> >> On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
> >>>
> >>> Looks like it assumes a pull request as properly reviewed as soon as
> >>> it gets a single approve - independent on how many reviewers are
> >>> required, see f.i.
> >>>
> >>> https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
> >>> https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
> >>>
> >>> -- Jeanette
> >>>
> >
> >
> >
>
>

Re: Skara - bot sending can-be-integrated message prematurely?

[Kevin Rushforth]

That's a good question about whether we can ask for a slight rewording 
of the Skara bot message to indicate that there might be other things to 
check before "/integrate". I'll raise that question with the Skara team.


As an aside, I noticed the other day that you typed "/integrate" after a 
single review, but in that case, there was no clear indication from Ajit 
(the first reviewer and the sponsor) that a second review was required, 
so I didn't comment on it.


-- Kevin


On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:


Kevin,

thanks for the clarification :) My bad that I didn't re-read the 
contrib.md. But then, who does? The lazy like myself do it 
occasionally only (down to once and then forget about it )


Maybe the bot message can be improved? With some indication that its 
(the bot's) knowledge about review requirements is limited, so would 
require a careful check by the contributor before actually post the 
/integrate comment? Actually, I think I goofed the other day, was 
safed only by Ajit who waited for the 2nd review before his /sponsor.


-- Jeanette

Zitat von Kevin Rushforth :

I added a comment to the two PRs in question, but it bears discussion 
here.


The Skara bot can't know whether all criteria have been met. It 
can't, for example, know whether there are outstanding comments from 
some reviewers that need to be addressed. Nor does it know which PRs 
need two reviewers (or sometimes a third if there is a specific 
person we would like to review it), which ones need a CSR, etc.


So having it state authoritatively that the PR is ready to integrate 
is a bit misleading. This is documented in the Code Review section of 
the CONTRIBUTING [1] doc:


NOTE: while the Skara tooling will indicate that the PR is ready to 
integrate once the first reviewer with a "Reviewer" role in the 
project has approved it, this may or may not be sufficient depending 
on the type of fix. For example, you must wait for a second approval 
for enhancements or high-impact bug fixes.


If anyone can think of a way to improve this and make it more clear, 
that would be helpful.


-- Kevin

[1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md


On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:


Looks like it assumes a pull request as properly reviewed as soon as 
it gets a single approve - independent on how many reviewers are 
required, see f.i.


https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: Skara - bot sending can-be-integrated message prematurely?

[Jeanette Winzenburg]

Kevin,

thanks for the clarification :) My bad that I didn't re-read the  
contrib.md. But then, who does? The lazy like myself do it  
occasionally only (down to once and then forget about it )


Maybe the bot message can be improved? With some indication that its  
(the bot's) knowledge about review requirements is limited, so would  
require a careful check by the contributor before actually post the  
/integrate comment? Actually, I think I goofed the other day, was  
safed only by Ajit who waited for the 2nd review before his /sponsor.


-- Jeanette

Zitat von Kevin Rushforth :


I added a comment to the two PRs in question, but it bears discussion here.

The Skara bot can't know whether all criteria have been met. It  
can't, for example, know whether there are outstanding comments from  
some reviewers that need to be addressed. Nor does it know which PRs  
need two reviewers (or sometimes a third if there is a specific  
person we would like to review it), which ones need a CSR, etc.


So having it state authoritatively that the PR is ready to integrate  
is a bit misleading. This is documented in the Code Review section  
of the CONTRIBUTING [1] doc:


NOTE: while the Skara tooling will indicate that the PR is ready to  
integrate once the first reviewer with a "Reviewer" role in the  
project has approved it, this may or may not be sufficient  
depending on the type of fix. For example, you must wait for a  
second approval for enhancements or high-impact bug fixes.


If anyone can think of a way to improve this and make it more clear,  
that would be helpful.


-- Kevin

[1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md


On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:


Looks like it assumes a pull request as properly reviewed as soon  
as it gets a single approve - independent on how many reviewers are  
required, see f.i.


https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: Skara - bot sending can-be-integrated message prematurely?

[Kevin Rushforth]

I added a comment to the two PRs in question, but it bears discussion here.

The Skara bot can't know whether all criteria have been met. It can't, 
for example, know whether there are outstanding comments from some 
reviewers that need to be addressed. Nor does it know which PRs need two 
reviewers (or sometimes a third if there is a specific person we would 
like to review it), which ones need a CSR, etc.


So having it state authoritatively that the PR is ready to integrate is 
a bit misleading. This is documented in the Code Review section of the 
CONTRIBUTING [1] doc:


NOTE: while the Skara tooling will indicate that the PR is ready to 
integrate once the first reviewer with a "Reviewer" role in the 
project has approved it, this may or may not be sufficient depending 
on the type of fix. For example, you must wait for a second approval 
for enhancements or high-impact bug fixes.


If anyone can think of a way to improve this and make it more clear, 
that would be helpful.


-- Kevin

[1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md


On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:


Looks like it assumes a pull request as properly reviewed as soon as 
it gets a single approve - independent on how many reviewers are 
required, see f.i.


https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: Skara - bot sending can-be-integrated message prematurely?

[Jeanette Winzenburg]

Zitat von Nir Lisker :


The bot doesn't know how many reviewers are needed. This was brought up
before in point 3 in
http://mail.openjdk.java.net/pipermail/openjfx-dev/2019-October/023662.html.



missed that - thanks for the pointer :)


- Nir

On Mon, Dec 16, 2019 at 2:24 PM Jeanette Winzenburg 
wrote:



Looks like it assumes a pull request as properly reviewed as soon as
it gets a single approve - independent on how many reviewers are
required, see f.i.

https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: Skara - bot sending can-be-integrated message prematurely?

[Nir Lisker]

The bot doesn't know how many reviewers are needed. This was brought up
before in point 3 in
http://mail.openjdk.java.net/pipermail/openjfx-dev/2019-October/023662.html.

- Nir

On Mon, Dec 16, 2019 at 2:24 PM Jeanette Winzenburg 
wrote:

>
> Looks like it assumes a pull request as properly reviewed as soon as
> it gets a single approve - independent on how many reviewers are
> required, see f.i.
>
> https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
> https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
>
> -- Jeanette
>
>