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