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 
> <kevin.rushforth at oracle.com <mailto:kevin.rushforth at 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.race at oracle.com 
>>> <mailto:philip.race at 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.rushforth at oracle.com
>>>     <mailto:kevin.rushforth at 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.rushforth at oracle.com
>>>     <mailto:kevin.rushforth at 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
>>>     >>>>>
>>>     >>>
>>>     >>>
>>>     >>
>>>
>>