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