Gerrit

Can we have ESC decision on that matter?

[Monorail components: ESC]

I've added this to the agenda for the next ESC meeting.

We are deferring to the newly formed plugin working group to come up with criteria and then revisit this issue.

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

As per [1]'s review, this Issue can now be converted to the new /corresponding template [2].

[1] https://gerrit-review.googlesource.com/c/gerrit/+/243027
[2] https://bugs.chromium.org/p/gerrit/templates/detail?template=Core+Plugin+Request

Please provide as much of the following information as possible (also see
https://gerrit-review.googlesource.com/Documentation/dev-core-
plugins.html#criteria):

1. URL to plugin repo:

https://gerrit.googlesource.com/plugins/reviewers

2. URL to the plugin license:

Apache License Version 2.0
https://gerrit.googlesource.com/plugins/reviewers/+/refs/heads/master/LICENSE

3. Plugin Scope (describe which functionality the plugin provides and how
it integrates with Gerrit core, does it conflict with any functionality
from Gerrit core or other core plugins?):

The plugin allows adding reviewers during CL upload based on per project configuration.
Standard query language is supported, for example, for top level directories,
for specific files or stable branches some specific reviewers can be added.

It also currently allows to configure the reviewers from the UI.
It is also possible to configure the plugin behaviour without the UI (change configuration using git commands only).

It is questionable as whether or not the plugin scope could/should be a part of Gerrit core, though.

Not all ESC members rermember my request to add "Change Fabricator" plugin repository to gerrit.googlesource.com site.
That request was rejected by the project founder, with the justification: "The feature scope of new plugin: "Create new empty
change *must* be part of gerit core, obviously".

Similarly, one could argue, that the ability to add reveiwers on CL upload based on query language, should be a part of Gerrit core.
I'm fine with either way and could help to internalize the code from reveiwers plugin and make it part of Gerrit core instead.

4. Related Plugins (other plugins that provide similar functionality):

o reviewers-by-blame (arguably blame based reveiwer addition could also be part of Gerrit Core, obviously,
   alternatively, that feature could also be integrated in reviewers plugin and be confiurable)
o reviewassistant
o find-owners
o owners

4a. Why is this plugin preferred over the other plugins?

It's using standard query language to configure reviewers.

5. Relevance (explain why this plugin is relevant to a majority of the
Gerrit community):

Almoust every middle to big size project (millions LoC) would like to configure automagically reviewers addition based on some criteria.

6. Existing Users (Which parties/organizations/companies already use the
plugin? If known, since when and how many users?):

gerrit-review.googlesource.com
Axis

7. Code Quality: (Are there any known issues? Does the code
adhere to Gerrit's quality standards? What's the expected maintenance
overhead for the maintainers?):

Are there any known issues?
No

Does the code adhere to Gerrit's quality standards?
Yes

What's the expected maintenance overhead for the maintainers?
No overhead expected.

8. Scalability/Performance (How well does the plugin functionality scale
for large repos and hosts with many repos? How does it affect latency of
user
requests?):

Change upload could be slightly delayed due to running the query, adding the reviewers and sending notifications, though.

9. Test Coverage (How good is the test coverage? Which
functionality is not covered by tests? If there are missing tests, is there
a
documented manual test plan?):

There are plenty of tests for the backend part of this plugin.
For the frontend part there are no tests yet, unfortunately, and there are some known styling issues that would need to be addressed
to have the same Look & Feel as in gerrit core UI.

10. Plugin documentation (provide a link to the plugin documentation and
point out things that are not properly documented):

Rest-API:
https://gerrit.googlesource.com/plugins/reviewers/+/refs/heads/master/src/main/resources/Documentation/rest-api.md

Config:
https://gerrit.googlesource.com/plugins/reviewers/+/refs/heads/master/src/main/resources/Documentation/config.md

11. Ongoing/Planned Work (Are there larger features that are in-progress or
planned? Preferably provide links to issues in Monorail.):

No work scheduled yet. The plugin scope is stable for years.
It can be considered to add other configuration ideas to this plugin as well, e.g. borrow feature for adding the reviewers based on blame information.

12. Do the current plugin owners agree to transfer the plugin ownership to
the ESC (of course any contributions from them continue to be welcome)?

Yes.

(Note to ESC: Keep this issue open for at least 10 calendar days so that
everyone in the community has a chance to raise concerns.)

This issues is already opened for years ;-)

> there are some known styling issues that would need to be addressed to have the same Look & Feel as in gerrit core UI

Can you elaborate on the issues that need to be addressed?

>> there are some known styling issues that would need to be addressed to have the same Look & Feel as in gerrit core UI
>Can you elaborate on the issues that need to be addressed?

Navigate to Gerrit project: [1] and compare styles for "Create Change"
dialog in core with "Reviewers Config" dialog from reviewers plugin: [2].

[1] https://gerrit-review.googlesource.com/admin/repos/gerrit,commands
[2] https://imgur.com/a/d5XvorK

By comparing [1] and [2] it looks like we need the "touch" of a front-end dev with CSS skills ;-)

> 7. Code Quality: (Are there any known issues?

Hi, I'd like to attract attention to [1] which breaks Gerrit core functionality "when a change is ignored user must not receive emails".

[1] https://bugs.chromium.org/p/gerrit/issues/detail?id=11625&q=component%3Aplugins%3Ereviewers&can=2

[Empty comment from Monorail migration]

I filed a separate issue after looking into the code a bit. That was an AI that I took out of the ESC meeting: https://bugs.chromium.org/p/gerrit/issues/detail?id=12610

[Empty comment from Monorail migration]

This no longer blocked on the core plugin process.

[Empty comment from Monorail migration]

Same as Patrick, I also filed a separate issue for the part I had to cover: https://crbug.com/gerrit/12614.

It looks like nobody is currently actively working on this anymore, so I am removing this from the list of issues that are relevant for the ESC.

[Monorail components: -ESC plugins>reviewers]

[Empty comment from Monorail migration]

Edits were made to reflect the following in Monorail: auto-CCs.