The Skara team has implemented SKARA-217 [1] to allow for increasing the
number of reviewers before it will mark a PR as ready for integration.
Starting from now, any "R"eviewer (that is, someone with a Reviewer role
in the OpenJFX Project) can indicate that a PR needs 2 reviewers (or
perhaps even 3 in some cases if one of the reviewers desires additional
eyes on a risky PR), can issue this command in the PR:
/reviewers 2
I'll do that now for the ones that we've already identified as needing
multiple reviewers.
Thanks to Nir Lisker for suggesting the Skara improvement, and to the
Skara team for implementing it. Let me know if there are any questions.
-- Kevin
[1] https://bugs.openjdk.java.net/browse/SKARA-217
On 1/8/2020 10:19 AM, Nir Lisker wrote:
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
<kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>> 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
<kevin.rushfo...@oracle.com <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
<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
>>>>>
>>>
>>>
>>
