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