Re: More GitHub labels

[Dmitry Belyavsky]

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

[Dr. Matthias St. Pierre]

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

[Dr. Matthias St. Pierre]

> 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

[Dr Paul Dale]

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

[Dr. Matthias St. Pierre]

> 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

More GitHub labels

[Dr Paul Dale]

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 (this post posed by @slontis’s comment in 12826 
).

These would give a clear indication of what additional material is being 
expected as per CLA required and need rebase.

A “hold: code needed" seems less useful :)


Thoughts or should we just add them?


Pauli
-- 
Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
Phone +61 7 3031 7217
Oracle Australia