Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-delegates to claim interest in a series #507

Open
kuba-moo opened this issue Dec 5, 2022 · 4 comments · May be fixed by #588
Open

Allow non-delegates to claim interest in a series #507

kuba-moo opened this issue Dec 5, 2022 · 4 comments · May be fixed by #588

Comments

@kuba-moo
Copy link

kuba-moo commented Dec 5, 2022

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.

@stephenfin
Copy link
Member

stephenfin commented Apr 6, 2024

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

@stephenfin
Copy link
Member

stephenfin commented Apr 6, 2024

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

@kuba-moo
Copy link
Author

kuba-moo commented Apr 9, 2024

The Gerrit model sounds good! Doing expiry externally sounds fine as well.

@andrepapoti
Copy link

andrepapoti commented Apr 16, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants