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 <philip.r...@oracle.com
<mailto:philip.r...@oracle.com>> 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
<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<g>)
>>>
>>> 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<kevin.rushfo...@oracle.com
<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
>>>>>
>>>
>>>
>>
