Repository navigation
Allow non-delegates to claim interest in a series #507
Description
Activity
@andrepapoti I'm not sure #588 tackles this exact issue. @kuba-moo is effectively a review request feature that would expire once the review has taken place (i.e. a change in state). #588 seems to allow something similar but makes expiry time-based. Is there a reason you've opted for the latter approach?
Here's my suggestion for how this could work: we simply inherit Gerrit's model. If you look at e.g. https://review.opendev.org/c/openstack/manila/+/872171 (The REST API resource can be inspected locally via e.g.
curl -s https://review.opendev.org/changes/I155b4ce4b605720c8335d465124fd32cc973a737/detail | sed -n '2 p' | jq), you'll see there are two lists of users exposed: a "Reviewers" list and a "CC" list. The former is for people who have reviewed or plan to review something, while the latter is for observers that would like to know about e.g. state changes. Users can add themselves to either list and are automatically added under certain conditions. In addition, there is a third list - the "Attention Set" list - which is a subset of the "Reviewers" list and indicates which of the users in the "Reviewers" list are expected to do stuff. How this works is described here but importantly there's no explicit time-based aspect to expiration or removal of a user from this set: it's based purely on a user action or a change in state on the patch.This feature works very well in my (pretty extensive) experience using it, and I suspect it would resolve most of what I expect you want from this feature with the benefit of avoiding additional new components like Celery. wdyt?
PS: If you wanted to remove users on a time basis to e.g. support some tooling you have, there's no reason this couldn't be API driven. We could also tackle it via
cron, like we do for sending emails right now.Reacted by Jakub KHow this would interface with delegates also remains to be seen. We might want to simply consider them as related but separate from reviewers (and CC) and think about them more as approvers. fwiw, this is the model prow and tide (k8s' CI and merge system) uses, though of course they allow for multiple approvers (read: delegates).
The Gerrit model sounds good! Doing expiry externally sounds fine as well.
I don't think going 1:1 with Gerrit would make much sense since we are using mailing lists there would not be much point on having the CC list since all of the people subscribed to the mailing lists are already getting notified whenever a new patch comes in.
The current Implementation already removed the user from the "Reviewers" list if it sent any comments for that patch; But I've changed the code to make the naming of the fields more clear.
In addition to that reviewers are considered as stale if they take more time than it's specified in the Patch state to make their review. (Resulting in something very similar to the Attention Set)
There's an attribute on the Patch that indicates if that are any users planning to review it, without considering the stale ones.
To prevent having to iterate over this list every time we request a Patch to check if there are any stale reviewers, the attribute is update asynchronously. I've updated the code to use cron jobs instead of celery to take care of it.
Currently maintainers have no idea if patch is in some reviewer's TODO / interest queue. Add support for non-maintianer users to mark patches as "review coming". Record time of marking so that stale marks can be removed. Automatically remove the marking if a review tag or comment arrives from a given person.