I like that suggestion. I'll run it by the Skara team.
-- Kevin
On 12/17/2019 4:50 PM, Scott Palmer wrote:
Perhaps the language in the message should be tweaked?
Maybe use, “This change can be integrated if there are no further review
requirements.“
Scott
On Dec 17, 2019, at 4:57 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> wrote:
Yes, other projects have multiple review requirements. And similar to JavaFX, the answer
to how many reviewers are needed is "it depends"; sometimes one is enough for
certain sub-projects or simple fixes; other sub-projects want two reviewers; sometimes
you want a specific expert to review a tricky change in one area being touched by a fix
in addition to the two primary reviewers.
I think if Skara tries to get into the business of covering all cases, we will
be adding a lot of complexity and still not be satisfied. The current approach,
which I think is the right one for now, is to for the tool to enforce the
minimum requirement for any fix to be integrated. Beyond that it is up to the
Committer and Reviewer(s) to make sure that the all criteria for integrating a
fix have been met.
-- Kevin
On 12/16/2019 4:14 PM, Nir Lisker wrote:
Do other projects also have multi-reviewers requirements?
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.
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
>>>
>
>
>
