Obsolete
Status Update
No update yet.
Comments
al...@google.com
[Empty comment from Monorail migration]
al...@google.com
[Empty comment from Monorail migration]
da...@gmail.com
[Empty comment from Monorail migration]
da...@gmail.com
[Empty comment from Monorail migration]
th...@gmail.com
Just for clarification (not that it changes the result of the analysis, but still) : contrary to indications in the doc, the ReviewAssistant plugin is supported on recent versions. We have been using it on 2.12, 2.14, 2.16, and I started work to fix backend changes in 3.x. Some work will also be needed to port to polygerrit 2.
Also, feature-wise reviewassistant plugin is more similar to reviewers-by-blame: e.g. it provides automatic reviewer assignement, as opposed to the "statically" selected reviewers of the reviewers plugin. In our setup, we actually use both the reviewers plugin, for static assignments, and either reviewers-by-blame or reviewassistant for ensuring there are always reviewers, with no configuration needed.
Also, feature-wise reviewassistant plugin is more similar to reviewers-by-blame: e.g. it provides automatic reviewer assignement, as opposed to the "statically" selected reviewers of the reviewers plugin. In our setup, we actually use both the reviewers plugin, for static assignments, and either reviewers-by-blame or reviewassistant for ensuring there are always reviewers, with no configuration needed.
al...@google.com
Thanks for the clarification regarding the reviewassistant plugin. Feature-wise, I would like to see one plugin evolving which concentrates on automatic reviewer addition with several modes to avoid the need for combining several similar, potentially conflicting plugins to cover all necessary flavors/use cases as you currently need to do.
da...@gmail.com
One of the questions during our discussion in the ESC meeting was "Why the reviewers plugin rather than any of the other similar plugins". We discussed the "owners" plugin and "find-owners" but it was pointed out that their use case is different.
I don't remember how much we discussed "review-assistant" and "reviewers-by-blame".
The main plus points in favor of "reviewers" were that it is actively maintained (in terms of keeping it buildable for current API versions) and is used on gerrit-review.
Perhaps we should take a step back and consider whether the "review-assistant" plugin would be a better fit than "reviewers"?
I don't remember how much we discussed "review-assistant" and "reviewers-by-blame".
The main plus points in favor of "reviewers" were that it is actively maintained (in terms of keeping it buildable for current API versions) and is used on gerrit-review.
Perhaps we should take a step back and consider whether the "review-assistant" plugin would be a better fit than "reviewers"?
sv...@axis.com
Short comments and reflection from working with the reviewers plugin on occasion:
Wouldn't it be useful to bre3ak all these points into separate issues and link them to this issue?
1. I concur. Is anyone using the "suggests" feature today?
2a. Ignoring WIP setting defaults to True so this is basically done already. If we decide to keep this configuration option. Personally I see no use-case for adding reviewers automatically to WIP changes.
2b. I concur and it's only a matter of removing the configuration once the decision is taken. [https://bugs.chromium.org/p/gerrit/issues/detail?id=12647 ]
Playing the devils advocate: To which lengths should we go to prevent Gerrit admins and gerrit-users (talking about installations in the wild here not gerrit-review) from (mis-)using features in other ways than they are intended to be used. One could argue that decent documentation is enough for Gerrit admins to make up their own mind of whether they should do something potentially hazardous or not.
3 and 4. Only Gerrit internal groups are supported. SingleUser plugin groups are f.i. not. [https://bugs.chromium.org/p/gerrit/issues/detail?id=12648 ]
AFAICT there is no limitations as to how many users the group(s) can contain (how many are attempted to be added as reveiwers).
If f.i. the username changes then all occurences in reviewers.configs would have to be updated manually.
If there will be a group-only limitation, reviewers plugin would need to support singleusergroup plugin groups.
The use-case is quite strong for having the flexibility to add single users without having multiple groups for each repo. You would practically need to have a group for each filter-section in the config, and some of our repositories have quite a lot of filter-sections in the reveiwer.config.
5-10. The REST API is in need of a redesign. I took the liberty of Copying your comments on 5-10 to an issue for redesigning the REST API [https://bugs.chromium.org/p/gerrit/issues/detail?id=12649 ]
11. AFAICT there's no filter validation. [https://bugs.chromium.org/p/gerrit/issues/detail?id=12650 ]
Wouldn't it be useful to bre3ak all these points into separate issues and link them to this issue?
1. I concur. Is anyone using the "suggests" feature today?
2a. Ignoring WIP setting defaults to True so this is basically done already. If we decide to keep this configuration option. Personally I see no use-case for adding reviewers automatically to WIP changes.
2b. I concur and it's only a matter of removing the configuration once the decision is taken. [
Playing the devils advocate: To which lengths should we go to prevent Gerrit admins and gerrit-users (talking about installations in the wild here not gerrit-review) from (mis-)using features in other ways than they are intended to be used. One could argue that decent documentation is enough for Gerrit admins to make up their own mind of whether they should do something potentially hazardous or not.
3 and 4. Only Gerrit internal groups are supported. SingleUser plugin groups are f.i. not. [
AFAICT there is no limitations as to how many users the group(s) can contain (how many are attempted to be added as reveiwers).
If f.i. the username changes then all occurences in reviewers.configs would have to be updated manually.
If there will be a group-only limitation, reviewers plugin would need to support singleusergroup plugin groups.
The use-case is quite strong for having the flexibility to add single users without having multiple groups for each repo. You would practically need to have a group for each filter-section in the config, and some of our repositories have quite a lot of filter-sections in the reveiwer.config.
5-10. The REST API is in need of a redesign. I took the liberty of Copying your comments on 5-10 to an issue for redesigning the REST API [
11. AFAICT there's no filter validation. [
sv...@axis.com
da...@gmail.com
> If f.i. the username changes then all occurences in reviewers.configs would have to be updated manually.
I'm not sure if this is really an issue, since IIRC the username cannot be changed once set.
I'm not sure if this is really an issue, since IIRC the username cannot be changed once set.
sv...@axis.com
sv...@axis.com
> I'm not sure if this is really an issue, since IIRC the username cannot be changed once set.
True.
That is what I have been assuming as well, wasn't sure that I recollected correctly :-)
True.
That is what I have been assuming as well, wasn't sure that I recollected correctly :-)
is...@google.com
Edits were made to reflect the following in Monorail: auto-CCs.
ae...@gmail.com
Comment has been deleted.
Description
*General feature suitability*
In general, automatic reviewer addition would be a desired feature for Gerrit and nicely extend Gerrit's existing feature set. It's imaginable that many users would benefit from it. Hence, having such a functionality as core plugin or even directly in core would make sense.
This doesn't cover additional functionality related to reviewers, though.
*Comparison with other plugins*
The original request named 'reviewers-by-blame', 'reviewerassistant', 'find-owners', and 'owners' as similar plugins.
* 'find-owners' and 'owners' have a different focus as they enforce specific approvals. Any automatic reviewer addition by them would be just a nice-to-have.
* 'reviewers-by-blame' seems to be very specific. As the request pointed out, the 'reviewers' plugin could be extended in the future to also support other ways to identify suitable reviewers for automatic addition.
* 'reviewassistant' would have many overlapping features with 'reviewers' as it also has a strong part about automatically adding reviewers. It even seems to offer different modes and considerations for it, which seems nice. On the other hand, that plugin is developed against Gerrit v2.10-rc0 according to its documentation and doesn't support other versions. Furthermore, 'reviewassistant' contains functionality unrelated to reviewer addition. That functionality part might also not be desired by all users.
*Feature and REST API improvement areas for the 'reviewers' plugin*
There are some aspects which would need to be clarified and fixed/adjusted before we can make 'reviewers' a core plugin.
1) The 'reviewers' plugin allows to switch to a reviewer-suggestion mode instead. This kind of functionality brings in a totally different aspect to the plugin compared to the otherwise reviewer-addition focus. We don't want to mix those two aspects at the moment. The 'reviewers' plugin should only focus on reviewer addition, which is a big enough area in its own. Any reviewer suggestions should be left out.
2) The 'reviewers' plugin should work nicely together with existing core Gerrit functionality. This is not always given.
2a) WIP changes shouldn't get reviewers added on upload. Only when a change switches out of the WIP state, reviewers should be automatically added and notified. For that case, it might also be desirable to show to users an indication that reviewers will be automatically added when they actively leave the WIP state on the UI. This is the default behavior the plugin should offer. If there are known use cases for a different desired behavior for WIP states, it would be good to document them so that we can consider whether offering a configuration option for another behavior would make sense.
2b) Private changes shouldn't get reviewers added automatically, not even via a config option. As private changes deliberately have a limited visibility, automatic reviewer addition could impose a risk, especially as the added reviewers are not transparent for a person uploading a change. To ensure that appropriate approvals/reviews are given for private changes, another approach (e.g. labels) has to be chosen. Ensuring appropriate approvals/reviews is not considered a responsibility of the 'reviewers' plugin. When a private change is made public, the automatic reviewer addition should kick in similarly as with switching out of the WIP state (-> ideally also with a UI indication).
3) The group support and behavior is not clear. Are only Gerrit internal groups supported? Is the name in the configuration as mentioned in the documentation the one from the groups file or the actual name of the group? If the latter, is renaming of groups considered? Is specification of other group identifiers (e.g. UUIDs) blocked? How can groups be distinguished from accounts? Will groups fully expand to their members? Will the expansion also happen transitively? What happens in case of very large groups? If a large number of reviewers is added due to such groups, it wouldn't be acceptable behavior, especially if such additions aren't and can't be limited. Overall, support for groups would either need to be polished or removed.
4) The documentation claims that email addresses or usernames can be used for specifying accounts. Are the other options (e.g. full name) blocked? If a user changes their username or email address, how will/can all occurrences in the 'reviewers' configuration be updated? Would this be manual or automatic? In the light of this, it might also make sense to switch to a group-only configuration as Gerrit deliberately only works with groups in other parts (e.g. permissions) to avoid issues related to persons changing names/identities.
5) The REST API of the plugin seems to be for configuration purposes. It's not clear why it's possible to deactivate the REST API of the plugin, especially as it would be necessary to support the UI part of the plugin.
6) Modification of reviewer configurations seems to be possible for project owners and people having a dedicated, global permission. The use cases for the global permission are not clear.
7) According to the documentation, only the PUT REST endpoint seems to be guarded by permissions. Having the GET REST endpoint completely open without any permissions could impose a risk, especially as groups and accounts could be mentioned which are not visible to all users.
8) Using just the name of the plugin as name for the REST API endpoints doesn't seem ideal. By having the endpoints "GET/PUT /projects/<project-name>/reviewers", the endpoints are claiming to be generally about reviewers, which they currently aren't. The focus of the 'reviewers' plugin is assumed to be on automatic reviewer addition. If so, the endpoints should reflect that by getting a different, targeted name. If the plugin and those endpoints are instead designed to cover various reviewer-related aspects (e.g. also suggestions), we'd need to evaluate the core plugin request in a different light.
9) The current REST API endpoints don't follow Gerrit's model for REST resources. For instance, a PUT would mean a complete update of a REST resource. The current PUT endpoint should be a POST endpoint instead. Furthermore, it's strange to use the same REST endpoint name for PUT/POST and GET without mirroring input and output. If those endpoints are not a simple write and read of each other, they should have different names. In addition, they should be updated to follow the REST-resource-model-style. Furthermore, the usage of the 'action' type fields seems to be not custom for Gerrit. If there is a core functionality which was used as inspiration, it would be good to know. If this is a new way, we should discuss the approach.
10) Inheritance of the configured default reviewers and interaction with the offered REST endpoints is not clear. Apparently, the configuration can be inherited from parent projects. Would the GET REST endpoint also mention the inherited default reviewers? Would the PUT/POST endpoint also allow to modify those? If no for both of them (which should be documented), does the UI at least point to the parent project configuration via a link as Gerrit currently does it for ACL configuration?
11) Is there a way to validate the configured 'filters' for valid queries? Is this done automatically on update? What are the effects of invalid configurations?