Respect the suggest.skipServiceUsers config when suggesting code owners
By default Gerrit doesn't suggest service users, but this behavior can
be overriden by setting suggest.skipServiceUsers=false in the gerrit
config. When this setting was added the code-owners plugin was not
adapted to respect it. Fix this now.
Bug: Google b/352467354
Change-Id: I234fddf7386979e0848abd911cca9d85281e15fa
Signed-off-by: Edwin Kempin <ekempin@google.com>
Reviewed-on: https://gerrit-review.googlesource.com/c/plugins/code-owners/+/433118
Reviewed-by: Nitzan Gur-Furman <nitzan@google.com>
Tested-by: Zuul <zuul-63@gerritcodereview-ci.iam.gserviceaccount.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index b7149c5..bcd0072 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -39,14 +39,17 @@
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.restapi.change.SuggestReviewers;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.Optional;
import java.util.function.Predicate;
import java.util.stream.Stream;
+import org.eclipse.jgit.lib.Config;
/**
* REST endpoint that gets the code owners for an arbitrary path in a revision of a change.
@@ -59,10 +62,12 @@
public class GetCodeOwnersForPathInChange
extends AbstractGetCodeOwnersForPath<CodeOwnersInChangeCollection.PathResource> {
+ private final Config cfg;
private final ServiceUserClassifier serviceUserClassifier;
@Inject
GetCodeOwnersForPathInChange(
+ @GerritServerConfig Config cfg,
AccountVisibility accountVisibility,
Accounts accounts,
AccountControl.Factory accountControlFactory,
@@ -87,6 +92,7 @@
codeOwnerResolver,
codeOwnerJsonFactory,
codeOwnerConfigFileJson);
+ this.cfg = cfg;
this.serviceUserClassifier = serviceUserClassifier;
}
@@ -222,6 +228,13 @@
}
private Predicate<CodeOwner> filterOutServiceUsers(ImmutableList.Builder<String> debugLogs) {
+ if (!cfg.getBoolean(
+ "suggest", "skipServiceUsers", SuggestReviewers.DEFAULT_SKIP_SERVICE_USERS)) {
+ // Returning true from the Predicate here means that the code owner should not be filtered
+ // out.
+ return codeOwner -> true;
+ }
+
return codeOwner -> {
if (!isServiceUser(codeOwner)) {
// Returning true from the Predicate here means that the code owner should be kept.
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
index a4d69f4..7d60771 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -300,6 +300,37 @@
}
@Test
+ @GerritConfig(name = "suggest.skipServiceUsers", value = "false")
+ public void codeOwnersThatAreServiceUsersAreNotFilteredOutIfSkippingServiceUsersIsDisabled()
+ throws Exception {
+ TestAccount serviceUser =
+ accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
+
+ // Create a code owner config with 2 code owners.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(serviceUser.email())
+ .create();
+
+ // Make 'serviceUser' a service user.
+ groupOperations
+ .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
+ .forUpdate()
+ .addMember(serviceUser.id())
+ .update();
+
+ // Check that both code owners are suggested, i.e. the service user is not filtered out.
+ assertThat(queryCodeOwners("/foo/bar/baz.md"))
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), serviceUser.id());
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "service.user@example.com")
public void globalCodeOwnersThatAreServiceUsersAreFilteredOut() throws Exception {
TestAccount serviceUser =
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index c97ba2c..ace28a0 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -742,7 +742,9 @@
The following code owners are filtered out additionally:
-* [service users](#serviceUsers) (members of the `Service Users` group)
+* [service users](#serviceUsers) (members of the `Service Users` group), unless
+ Gerrit is [configured](../../../Documentation/config-gerrit.html#suggest.skipServiceUsers)
+ to not skip service users
* the change owner (since the change owner cannot be added as reviewer)
* code owners that are annotated with
[LAST_RESORT_SUGGESTION](backend-find-owners.html#lastResortSuggestion),
