> > 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.r...@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.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 > >>>>> > >>> > >>> > >> >
