Re: More GitHub labels
On Thu, Sep 10, 2020 at 9:20 AM Dr. Matthias St. Pierre < matthias.st.pie...@ncp-e.com> wrote: > > ... I think we should change that. This does not mean that a reviewer > who made a change request > > two months ago and lost interest is forced to re-review, only that such > stale reviews must be dismissed > > explicitly, if the reviewer does not respond to a re-review request > within a certain time period. > > I would refine this procedure as follows: the team member who intends to > do the merge (the "merger"), > needs to issue re-review requests for all unresolved change requests > (there is a spinning button next the > name of the reviewer to do this). The person who receives the re-review > request can either dismiss its > review or indicate that it intends to review within x hours. Otherwise, > the merger can dismiss the stale review. > > Sorry, it seems a bit overengineering for me. I'd prefer a procedure with explicit hold and explanation in the comments. -- SY, Dmitry Belyavsky
RE: More GitHub labels
> ... I think we should change that. This does not mean that a reviewer who > made a change request > two months ago and lost interest is forced to re-review, only that such stale > reviews must be dismissed > explicitly, if the reviewer does not respond to a re-review request within a > certain time period. I would refine this procedure as follows: the team member who intends to do the merge (the "merger"), needs to issue re-review requests for all unresolved change requests (there is a spinning button next the name of the reviewer to do this). The person who receives the re-review request can either dismiss its review or indicate that it intends to review within x hours. Otherwise, the merger can dismiss the stale review. Matthias
RE: More GitHub labels
> Your suggestion seems workable too. PRs are merged with outstanding change > requests indicated > — a reviewer comments, the comments are addressed then a different reviewer > approves without > the original review being removed. The labels are a bit more in your face. > A hybrid “hold: required changes see review/comments” might be workable. GitHub can be configured to require approval. It then gives a clear visual indication that the PR is not ready to be merged, and the Merge button is greyed out. This should be obvious enough, even more obvious than a label, which can also easily be ignored. Of course, our merge procedure circumvents the merge button, I'm only talking about the visual indicator. Alternatively, the pre-receive-hook on the git.openssl.org server could enforce the policy by checking the reviewer state via GitHub API queries. Matthias
Re: More GitHub labels
Matthias, Your suggestion seems workable too. PRs are merged with outstanding change requests indicated — a reviewer comments, the comments are addressed then a different reviewer approves without the original review being removed. The labels are a bit more in your face. A hybrid “hold: required changes see review/comments” might be workable. Pauli -- Dr Paul Dale | Distinguished Architect | Cryptographic Foundations Phone +61 7 3031 7217 Oracle Australia > On 10 Sep 2020, at 4:03 pm, Dr. Matthias St. Pierre > wrote: > >> Just wondering if we should have two new labels: “hold: tests needed” and >> “hold: documentation needed” labels? >> There are a number of PRs that come through where one or both of these are >> missing missing. > > The two use cases you mention are actually better handled by a change request > (via GitHub review) instead of a label: > A team member can formulate a change request to block the merging. Here he > has more freedom to formulate what > needs to be done. This includes your two use cases, as well as the use case > for the label [hold: need rebase]. > > The problem with it is that currently we count only approvals and don’t > consider unresolved change requests > as a blocker. I think we should change that. This does not mean that a > reviewer who made a change request > two months ago and lost interest is forced to re-review, only that such stale > reviews must be dismissed > explicitly, if the reviewer does not respond to a re-review request within a > certain time period. > > Matthias >
RE: More GitHub labels
> Just wondering if we should have two new labels: “hold: tests needed” and > “hold: documentation needed” labels? > There are a number of PRs that come through where one or both of these are > missing missing. The two use cases you mention are actually better handled by a change request (via GitHub review) instead of a label: A team member can formulate a change request to block the merging. Here he has more freedom to formulate what needs to be done. This includes your two use cases, as well as the use case for the label [hold: need rebase]. The problem with it is that currently we count only approvals and don’t consider unresolved change requests as a blocker. I think we should change that. This does not mean that a reviewer who made a change request two months ago and lost interest is forced to re-review, only that such stale reviews must be dismissed explicitly, if the reviewer does not respond to a re-review request within a certain time period. Matthias