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 <kevin.rushforth at 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.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>
> 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>
>> > 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>:
>> >>>
>> >>>> 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
>> >>>>>
>> >>>
>> >>>
>> >>
>>
>
>
>