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