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