Per discussion in T91190, it would be nice to enable reviewers-by-blame which helps inexperienced developers find the right person for code review.
(I'm not sure what the roadmap is for Gerrit -> Phabricator migration - please decline if it's too close for Gerrit improvements to be worth the effort. Closest Phabricator equivalent I have found is phabricator:T1743, still under development.)
Description
Description
Details
Details
Customize query in gerrit
Related Objects
Related Objects
- Mentioned In
- T333010: [Session] Using automated systems to recommend reviewers in the MediaWiki project
rGERRITDEPLOY3cd0d7352b08: Update plugins reviewers-by-blame to stable-2.15
rGERRITDEPLOY0817b93b342a: Remove reviewers-by-blame from deployment
rGERRITDEPLOYffe3882bdc60: Remove reviewers-by-blame
rGERRITDEPLOY7d5ffdfd88fb: Remove reviewers-by-blame
rGERRITDEPLOY6cdece9f38d3: Remove reviewers-by-blame from deployment
T207984: Metrics that reduce workload on human reviewers
Blog Post: Gerrit now automatically adds reviewers
rGERRITDEPLOY39bfd194c64b: Update plugins reviewers-by-blame to stable-2.15
rGERRITDEPLOY660bbfeb1b29: Adding reviewers-by-blame plugin
T155851: AI which suggests best reviewers for a patch ("Patch wrangler")
T91190: When uploading a new patch, reviewers should be added automatically - Mentioned Here
- T210785: Upgrade Gerrit to 2.15.8
T91190: When uploading a new patch, reviewers should be added automatically
Event Timeline
Comment Actions
This project is selected for the Developer-Wishlist voting round and will be added to a MediaWiki page very soon. To the subscribers, or proposer of this task: please help modify the task description: add a brief summary (10-12 lines) of the problem that this proposal raises, topics discussed in the comments, and a proposed solution (if there is any yet). Remember to add a header with a title "Description," to your content. Please do so before February 5th, 12:00 pm UTC.
Comment Actions
This will have to be written for polygerrit's ui since gwtui has begun to be removed upstream. We can still make it for gwtui but it will need to support polygerrit ui too.
Comment Actions
Change 422178 had a related patch set uploaded (by Chad; owner: Chad):
[operations/software/gerrit/gerrit@wmf/stable-2.14] Adding reviewers-by-blame plugin
Comment Actions
Change 422178 merged by Chad:
[operations/software/gerrit/gerrit@wmf/stable-2.14] Adding reviewers-by-blame plugin
Comment Actions
We have the plugin installed: https://gerrit.wikimedia.org/r/plugins/reviewers-by-blame/Documentation/index.html as well as the reviewer one https://gerrit.wikimedia.org/r/plugins/reviewers/Documentation/index.html
Both can be configured on a per project basis by the project owners. Ideally one would want to craft a tutorial/example, highlight it via a Phame post / wikitech-l then I guess we can call it done.
Comment Actions
The plugin is enabled. On All-Projects there are some configuration bits available:
reviewers-by-blame Plugin
Ignore file Regex: ____________
[Ignore files that match the given regular expression when looking for reviewers]
Ignore Regex: ____________
[Ignore commits where the subject matches the given regular expression]
Max Reviewers: 3
[The maximum number of reviewers that should be automatically added to a change based on the git blame computation on the changed files.]
So it should work? Unless there is some magic flag required.
I have send a patch to restore logging of plugins at INFO level with https://gerrit.wikimedia.org/r/#/c/475226/ , that might give us some clues. Then if haven't figured it out yet, we can add a log4j debug logger for com.googlesource.gerrit.plugins.reviewersbyblame.
Comment Actions
https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/475226/ enables logging for plugins:
[2018-11-28 16:57:47,182] [main] INFO : Loaded plugin reviewers-by-blame, version 2c08ef7
Will probably need to be tweaked to use DEBUG level for the reviewersbyblame plugin (see above), will let us figure out whether it works properly.
Comment Actions
Change 476343 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/puppet@production] gerrit: log reviewers-by-blame plugin at DEBUG level
Comment Actions
Change 476343 abandoned by Hashar:
gerrit: log reviewers-by-blame plugin at DEBUG level
Reason:
Bah actually the code only has log entries for warn or error ...
I assume that is already logged by the root logger.
Comment Actions
Does not seem to be working, my list of reviewer suggestions seems to be populated from people I asked for a review in the past, and is not specific to the repo. E.g. here is a random extension: BlueSpiceExtendedFilelist, you can see that the top reviewer is Robert Vogel. But when I click on "add reviewer" I get a list of people who have never even interacted with that repo, and as far as I can see the only common feature is that they have all reviewed patches of mine in the past, and that I have asked all of them for a review at some point in time in some repo (but not BlueSpiceExtendedFilelist).
Comment Actions
Mentioned in SAL (#wikimedia-operations) [2019-01-08T09:19:39Z] <hashar> gerrit: resaved configuration for All-Projects by changing "Max Reviewers" from 3 to 4. Might enable adding reviewers automatically based on git blame. See task for config diff # T101131
Comment Actions
Indeed it is definitely not adding reviewers. That puzzled me during the xmas break and eventually had an idea this morning. Based on my previous comment about the plugin configuration ( T101131#4766853 ) I eventually looked at the raw configuration file on the project:
$ git clone https://gerrit.wikimedia.org/r/p/All-Projects.git $ git fetch refs/meta/config && git checkout FETCH_HEAD $ grep -i reviewer project.config $
Ie despite the UI showing some values, there are none listed in the All-Projects configuration. In the web UI I have thus changed Max Reviewers from 3 to 4 (using copy paste since my browser/javascript would not let me add a value). I have saved the config b972f5ea571e7bd15542a6562dd1d0b010681a1a which got me:
$ git fetch refs/meta/config
$ git reset --hard FETCH_HEAD
$ git show
commit b972f5ea571e7bd15542a6562dd1d0b010681a1a
Author: Hashar <hashar@free.fr>
Date: Tue Jan 8 09:17:04 2019 +0000
Modified project settings
diff --git a/project.config b/project.config
index fa884eb..85399de 100644
--- a/project.config
+++ b/project.config
@@ -51,6 +51,7 @@
[submit]
mergeContent = false
action = fast forward only
+ matchAuthorToCommitterDate = false
[dashboard]
default = refs/meta/dashboards/default:recent
[label "Verified"]
@@ -90,5 +91,12 @@
push = group Administrators
push = group Gerrit Managers
push = group Project Owners
+[change]
+ privateByDefault = false
+ workInProgressByDefault = false
+[reviewer]
+ enableByEmail = false
+[plugin "reviewers-by-blame"]
+ maxReviewers = 4
[plugin "its-phabricator"]
enabled = trueWhich is b972f5ea571e7bd15542a6562dd1d0b010681a1a. Some settings are just normalization. The code default to 3 anyway via a getInt("maxReviewers", 3). So I dont see how my change could modify anything.
Note the plugin is definitely installed and loaded based on https://gerrit.wikimedia.org/r/#/admin/plugins/
Comment Actions
Eventually I have found https://groups.google.com/d/msg/repo-discuss/-5kMTN34aqw/zgTAEgReDgAJ reviewers-by-blame on stable-2.15 busted with NoteDb. It points at Change 192110: Find change via ChangeData.Factory rather than ReviewDb.
Git log:
* 0b6233e - (origin/stable-2.15) Find change via ChangeData.Factory rather than ReviewDb (8 weeks ago) <Richard Christie> | * 5ff127d - (origin/master, origin/HEAD) Adapats plugin to changes done in core (3 months ago) <Paladox none> | * d2fc6d7 - Adapt to new core package layout (1 year, 2 months ago) <Paladox none> |/ * 2c08ef7 - (HEAD -> master) Adapt to removed AccountByEmailCache (1 year, 2 months ago) <Paladox none> * 85abed9 - Remove support for drafts (1 year, 2 months ago) <Paladox none> * 7daadf5 - (origin/stable-2.14) Format all Java files with google-java-format (1 year, 2 months ago) <David Pursehouse>
So we need to update our copy of reviewers-by-blame from 2c08ef7 to stable-2.15 and we will get the change.
Comment Actions
Change 482775 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/software/gerrit@wmf/stable-2.15] Update plugins reviewers-by-blame to stable-2.15
Comment Actions
From @thcipriani to rebuild the plugin one would do something like:
bazel build plugins/reviewers-by-blame wmf/stable-2.15 bazel build plugins/reviewers-by-blame:reviewers-by-blame
Result in bazel-bin
Upload the resulting .jar to Archiva, update git fat (we have a script to handle that: upload-artifacts script).
Then scap deploy. We don't even need to restart Gerrit.
Comment Actions
I went with install bazel from their apt package (which is a huge binary blob bah). Then:
$ cd ~/projects/operations/software/gerrit $ bazel build plugins/reviewers-by-blame:reviewers-by-blame INFO: Analysed target //plugins/reviewers-by-blame:reviewers-by-blame (142 packages loaded, 3182 targets configured). INFO: Found 1 target... Target //plugins/reviewers-by-blame:reviewers-by-blame up-to-date: bazel-genfiles/plugins/reviewers-by-blame/reviewers-by-blame.jar INFO: Elapsed time: 44.699s, Critical Path: 12.04s INFO: 126 processes: 121 processwrapper-sandbox, 5 worker. INFO: Build completed successfully, 140 total actions
Which gets me a ton of .jar files:
$ find bazel-genfiles/ -type f bazel-genfiles/gerrit-server/prolog-common.srcjar bazel-genfiles/gerrit-patch-jgit/edit.srcjar bazel-genfiles/external/bcprov/jar/_ijar/jar/external/bcprov/jar/bcprov-jdk15on-1.57-ijar.jar bazel-genfiles/external/mina-core/jar/_ijar/jar/external/mina-core/jar/mina-core-2.0.16-ijar.jar bazel-genfiles/external/automaton/jar/_ijar/jar/external/automaton/jar/automaton-1.11-8-ijar.jar bazel-genfiles/external/protobuf/jar/_ijar/jar/external/protobuf/jar/protobuf-java-3.0.0-beta-2-ijar.jar bazel-genfiles/external/commons-validator/jar/_ijar/jar/external/commons-validator/jar/commons-validator-1.6-ijar.jar bazel-genfiles/external/commons-net/jar/_ijar/jar/external/commons-net/jar/commons-net-3.5-ijar.jar bazel-genfiles/external/httpcore/jar/_ijar/jar/external/httpcore/jar/httpcore-4.4.1-ijar.jar bazel-genfiles/external/soy/jar/_ijar/jar/external/soy/jar/soy-2017-04-23-ijar.jar bazel-genfiles/external/mime-util/jar/_ijar/jar/external/mime-util/jar/mime-util-2.1.3-ijar.jar bazel-genfiles/external/ow2-asm-tree/jar/_ijar/jar/external/ow2-asm-tree/jar/asm-tree-5.1-ijar.jar bazel-genfiles/external/guava/jar/_ijar/jar/external/guava/jar/guava-22.0-ijar.jar bazel-genfiles/external/log-api/jar/_ijar/jar/external/log-api/jar/slf4j-api-1.7.7-ijar.jar bazel-genfiles/external/joda-time/jar/_ijar/jar/external/joda-time/jar/joda-time-2.9.9-ijar.jar bazel-genfiles/external/jgit-lib/jar/_ijar/jar/external/jgit-lib/jar/org.eclipse.jgit-4.9.7.201810191756-r-ijar.jar bazel-genfiles/external/gson/jar/_ijar/jar/external/gson/jar/gson-2.8.0-ijar.jar bazel-genfiles/external/commons-codec/jar/_ijar/jar/external/commons-codec/jar/commons-codec-1.10-ijar.jar bazel-genfiles/external/guice-library/jar/_ijar/jar/external/guice-library/jar/guice-4.2.0-ijar.jar bazel-genfiles/external/ow2-asm-util/jar/_ijar/jar/external/ow2-asm-util/jar/asm-util-5.1-ijar.jar bazel-genfiles/external/grappa/jar/_ijar/jar/external/grappa/jar/grappa-1.0.4-ijar.jar bazel-genfiles/external/lucene-analyzers-common/jar/_ijar/jar/external/lucene-analyzers-common/jar/lucene-analyzers-common-5.5.4-ijar.jar bazel-genfiles/external/pegdown/jar/_ijar/jar/external/pegdown/jar/pegdown-1.6.0-ijar.jar bazel-genfiles/external/lucene-misc/jar/_ijar/jar/external/lucene-misc/jar/lucene-misc-5.5.4-ijar.jar bazel-genfiles/external/tukaani-xz/jar/_ijar/jar/external/tukaani-xz/jar/xz-1.4-ijar.jar bazel-genfiles/external/dropwizard-core/jar/_ijar/jar/external/dropwizard-core/jar/metrics-core-4.0.3-ijar.jar bazel-genfiles/external/commons-lang/jar/_ijar/jar/external/commons-lang/jar/commons-lang-2.6-ijar.jar bazel-genfiles/external/velocity/jar/_ijar/jar/external/velocity/jar/velocity-1.7-ijar.jar bazel-genfiles/external/jgit-servlet/jar/_ijar/jar/external/jgit-servlet/jar/org.eclipse.jgit.http.server-4.9.7.201810191756-r-ijar.jar bazel-genfiles/external/icu4j/jar/_ijar/jar/external/icu4j/jar/icu4j-57.1-ijar.jar bazel-genfiles/external/ow2-asm-commons/jar/_ijar/jar/external/ow2-asm-commons/jar/asm-commons-5.1-ijar.jar bazel-genfiles/external/java-runtime/jar/_ijar/jar/external/java-runtime/jar/antlr-runtime-3.5.2-ijar.jar bazel-genfiles/external/gwtorm-client/jar/_ijar/jar/external/gwtorm-client/jar/gwtorm-1.18-ijar.jar bazel-genfiles/external/gwtorm-client/jar/_ijar/src/external/gwtorm-client/jar/gwtorm-1.18-src-ijar.jar bazel-genfiles/external/guice-servlet/jar/_ijar/jar/external/guice-servlet/jar/guice-servlet-4.2.0-ijar.jar bazel-genfiles/external/sshd/jar/_ijar/jar/external/sshd/jar/sshd-core-1.6.0-ijar.jar bazel-genfiles/external/guava-retrying/jar/_ijar/jar/external/guava-retrying/jar/guava-retrying-2.0.0-ijar.jar bazel-genfiles/external/httpclient/jar/_ijar/jar/external/httpclient/jar/httpclient-4.4.1-ijar.jar bazel-genfiles/external/jgit-archive/jar/_ijar/jar/external/jgit-archive/jar/org.eclipse.jgit.archive-4.9.7.201810191756-r-ijar.jar bazel-genfiles/external/user/jar/_ijar/jar/external/user/jar/gwt-user-2.8.2-ijar.jar bazel-genfiles/external/javax_inject/jar/_ijar/jar/external/javax_inject/jar/javax.inject-1-ijar.jar bazel-genfiles/external/mime4j-core/jar/_ijar/jar/external/mime4j-core/jar/apache-mime4j-core-0.8.0-ijar.jar bazel-genfiles/external/juniversalchardet/jar/_ijar/jar/external/juniversalchardet/jar/juniversalchardet-2.0.0-ijar.jar bazel-genfiles/external/commons-dbcp/jar/_ijar/jar/external/commons-dbcp/jar/commons-dbcp-1.4-ijar.jar bazel-genfiles/external/jsoup/jar/_ijar/jar/external/jsoup/jar/jsoup-1.9.2-ijar.jar bazel-genfiles/external/gwtjsonrpc/jar/_ijar/jar/external/gwtjsonrpc/jar/gwtjsonrpc-1.11-ijar.jar bazel-genfiles/external/gwtjsonrpc/jar/_ijar/src/external/gwtjsonrpc/jar/gwtjsonrpc-1.11-src-ijar.jar bazel-genfiles/external/ow2-asm/jar/_ijar/jar/external/ow2-asm/jar/asm-5.1-ijar.jar bazel-genfiles/external/ow2-asm-analysis/jar/_ijar/jar/external/ow2-asm-analysis/jar/asm-analysis-5.1-ijar.jar bazel-genfiles/external/dev/jar/_ijar/jar/external/dev/jar/gwt-dev-2.8.2-ijar.jar bazel-genfiles/external/log4j/jar/_ijar/jar/external/log4j/jar/log4j-1.2.17-ijar.jar bazel-genfiles/external/jsch/jar/_ijar/jar/external/jsch/jar/jsch-0.1.54-ijar.jar bazel-genfiles/external/guice-assistedinject/jar/_ijar/jar/external/guice-assistedinject/jar/guice-assistedinject-4.2.0-ijar.jar bazel-genfiles/external/h2/jar/_ijar/jar/external/h2/jar/h2-1.3.176-ijar.jar bazel-genfiles/external/jsonevent-layout/jar/_ijar/jar/external/jsonevent-layout/jar/jsonevent-layout-1.7-ijar.jar bazel-genfiles/external/lucene-queryparser/jar/_ijar/jar/external/lucene-queryparser/jar/lucene-queryparser-5.5.4-ijar.jar bazel-genfiles/external/eddsa/jar/_ijar/jar/external/eddsa/jar/eddsa-0.2.0-ijar.jar bazel-genfiles/external/auto-value-annotations/jar/_ijar/jar/external/auto-value-annotations/jar/auto-value-annotations-1.6.2-ijar.jar bazel-genfiles/external/bcpkix/jar/_ijar/jar/external/bcpkix/jar/bcpkix-jdk15on-1.57-ijar.jar bazel-genfiles/external/auto-value/jar/_ijar/jar/external/auto-value/jar/auto-value-1.6.2-ijar.jar bazel-genfiles/external/mime4j-dom/jar/_ijar/jar/external/mime4j-dom/jar/apache-mime4j-dom-0.8.0-ijar.jar bazel-genfiles/external/blame-cache/jar/_ijar/jar/external/blame-cache/jar/blame-cache-0.2-5-ijar.jar bazel-genfiles/external/prolog-runtime/jar/_ijar/jar/external/prolog-runtime/jar/prolog-runtime-1.4.3-ijar.jar bazel-genfiles/external/prolog-runtime/jar/_ijar/neverlink/external/prolog-runtime/jar/prolog-runtime-1.4.3-ijar.jar bazel-genfiles/external/commons-compress/jar/_ijar/jar/external/commons-compress/jar/commons-compress-1.13-ijar.jar bazel-genfiles/external/args4j/jar/_ijar/jar/external/args4j/jar/args4j-2.0.26-ijar.jar bazel-genfiles/external/servlet-api-3_1/jar/_ijar/jar/external/servlet-api-3_1/jar/tomcat-servlet-api-8.0.24-ijar.jar bazel-genfiles/lib/lucene/lucene-core-and-backward-codecs__merged.jar bazel-genfiles/lib/lucene/_ijar/lucene-core-and-backward-codecs/lib/lucene/lucene-core-and-backward-codecs__merged-ijar.jar bazel-genfiles/plugins/reviewers-by-blame/reviewers-by-blame.jar bazel-genfiles/gerrit-index/query_antlr.srcjar
Maybe we just need bazel-genfiles/plugins/reviewers-by-blame/reviewers-by-blame.jar
Comment Actions
Change 482775 abandoned by Hashar:
Update plugins reviewers-by-blame to stable-2.15
Reason:
Superseeded by the upgrade to Gerrit 2.15.8 https://gerrit.wikimedia.org/r/#/c/operations/software/gerrit/ /484437/
Comment Actions
The plugin is going to be bumped by https://gerrit.wikimedia.org/r/#/c/operations/software/gerrit/+/484437/-1..1
Comment Actions
The plugin version we were using was not compatible yet with Gerrit 2.15. In the previous upgrades we forgot to bump the plugin.
With Gerrit 2.15.8 (T210785) which we upgraded a few minutes ago, the plugin got bumped. Looking at a few recent changes, they seem to have magic reviewers being added to change!
I am assuming the plugin works properly now!
Comment Actions
Works great, thank you for fixing it!
Given that action of adding reviewers is done under the name of the uploader, so it's not entirely obvious what's going on, maybe a wikitech-l announcement would be in order?
(Or we could just ask upstream to add an option to use a dedicated user for adding reviers.)
Comment Actions
We should do both, Email in the short term, but ideally in the long term we should request automated actions like that are done by a system user and not as a actual user when the user isn't actually performing the action.
Comment Actions
I have published a blog explaining the Gerrit review by blame plugin as well as other manual way to find reviewers: J139: Blog Post: Gerrit now automatically adds reviewers
Comment Actions
@Paladox had filed https://bugs.chromium.org/p/gerrit/issues/detail?id=10318 which is similar (exempting bot accounts vs using a dedicated account)
Comment Actions
Change 485184 had a related patch set uploaded (by Hashar; owner: Hashar):
[All-Projects@refs/meta/config] reviewers-by-blame: default to not adding any reviewers
Comment Actions
As a follow up (also posted as a comment to J139):
The Gerrit plugin blindly adds reviewers even when in a few cases it might not be appropriate (for a bot account, author of code who is not qualified to do a review, user that is no more active on a repository).
As such, the plugin will no more report by default. The configuration setting maxReviewers has been set to 0 in All-Projects.git(Gerrit #485184) which causes the plugin to early abort. The setting is inherited by all repositories
Project owners can still enable the setting for their repository by editing the project settings and setting maxReviewers to a positive integer.
Comment Actions
Change 485184 merged by Thcipriani:
[All-Projects@refs/meta/config] reviewers-by-blame: default to not adding any reviewers
Comment Actions
More upstream reports:
- #10337 Support a "Suggest Reviewer" button in reviewers-by-blame (by @Paladox)
- #10340 Add review by blame of CR+2
- #10339 reviewers-by-blame should not readd reviewers who have removed themselves
- #10345 Option to limit reviewers-by-blame to new users
Also, Paladox is working on a per-user disabling patch: 210812 Support blacklisting users
Comment Actions
Change 485810 had a related patch set uploaded (by Hashar; owner: Thcipriani):
[operations/software/gerrit@deploy/wmf/stable-2.15] Remove reviewers-by-blame from deployment
Comment Actions
Thank you @Tgr and @Paladox . We are going to uninstall the plugin entirely based on the feedback from the thread wikitech-l Gerrit now automatically adds reviewers.
We would need way more features to be implement before considering enabling it again. Something to take in mind for later is to gradually enable it per repository instead of globally.
I am changing this to Declined, I don't think we will enable it anytime soon. But feel free to flip it to stalled state instead :)
Comment Actions
Change 485810 merged by Thcipriani:
[operations/software/gerrit@deploy/wmf/stable-2.15] Remove reviewers-by-blame from deployment
Comment Actions
Change 486262 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/software/gerrit@wmf/stable-2.15] Remove reviewers-by-blame
Comment Actions
Change 486262 merged by Thcipriani:
[operations/software/gerrit@wmf/stable-2.15] Remove reviewers-by-blame