feat(gerrit): add code-owners plugin
This is the New Thing that is intended to replace the find-owners and owners plugins. In particular: * It inserts a submit requirement rather than providing a Prolog predicate. * The default OWNERS file formats are suspiciously Googley. * It provides a neat UI for finding OWNERS and tracking approval state on a per-file basis. When we fully migrate to using the code-owners plugin, a few things will need to land, which I will likely do "offline" directly to the Gerrit backing Git repos: * Add the corresponding Gerrit config * Replace OWNERS files depot-wide * Add OWNERS files to the refs/meta/config branch * Introduce the Owners-Override label, settable by depot-interventions The enclosed patch adds two extra pieces of functionality that we need in tvldepot but aren't upstream: 1. The ability to just specify usernames rather than email addresses 2. The ability to specify `group:GROUPNAME`, _as long as_ that group is visible to everyone. This is a restriction intended to avoid having the plugin just leak group membership. Change-Id: I27d92b6cb7449af83030b9015f09a1571aa8452f Reviewed-on: https://cl.tvl.fyi/c/depot/+/6664 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: sterni <sternenseemann@systemli.org>
This commit is contained in:
Luke Granger-Brown
lukegb
parent
f358be4a1d
commit
ae98240df2
3 changed files with 453 additions and 7 deletions
18
third_party/gerrit_plugins/builder.nix
vendored
18
third_party/gerrit_plugins/builder.nix
vendored
|
|
@ -1,4 +1,4 @@
|
||||||
{ depot, pkgs, ... }:
|
{ depot, lib, pkgs, ... }:
|
||||||
{
|
{
|
||||||
buildGerritBazelPlugin =
|
buildGerritBazelPlugin =
|
||||||
{ name
|
{ name
|
||||||
|
|
@ -8,7 +8,7 @@
|
||||||
cp -R "${src}" "$out/plugins/${name}"
|
cp -R "${src}" "$out/plugins/${name}"
|
||||||
''
|
''
|
||||||
, postPatch ? ""
|
, postPatch ? ""
|
||||||
,
|
, patches ? [ ]
|
||||||
}: ((depot.third_party.gerrit.override {
|
}: ((depot.third_party.gerrit.override {
|
||||||
name = "${name}.jar";
|
name = "${name}.jar";
|
||||||
|
|
||||||
|
|
@ -26,10 +26,14 @@
|
||||||
installPhase = ''
|
installPhase = ''
|
||||||
cp "bazel-bin/plugins/${name}/${name}.jar" "$out"
|
cp "bazel-bin/plugins/${name}/${name}.jar" "$out"
|
||||||
'';
|
'';
|
||||||
postPatch =
|
postPatch = ''
|
||||||
if super ? postPatch then ''
|
${super.postPatch or ""}
|
||||||
${super.postPatch}
|
pushd "plugins/${name}"
|
||||||
${postPatch}
|
${lib.concatMapStringsSep "\n" (patch: ''
|
||||||
'' else postPatch;
|
patch -p1 < ${patch}
|
||||||
|
'') patches}
|
||||||
|
popd
|
||||||
|
${postPatch}
|
||||||
|
'';
|
||||||
}));
|
}));
|
||||||
}
|
}
|
||||||
|
|
|
||||||
17
third_party/gerrit_plugins/code-owners/default.nix
vendored
Normal file
17
third_party/gerrit_plugins/code-owners/default.nix
vendored
Normal file
|
|
@ -0,0 +1,17 @@
|
||||||
|
{ depot, pkgs, ... }@args:
|
||||||
|
|
||||||
|
let
|
||||||
|
inherit (import ../builder.nix args) buildGerritBazelPlugin;
|
||||||
|
in
|
||||||
|
buildGerritBazelPlugin rec {
|
||||||
|
name = "code-owners";
|
||||||
|
depsOutputHash = "sha256:0fpv5yavgki5nv84lg5zykp6v7pv9xll1glmz5dwnz5z11axj4g9";
|
||||||
|
src = pkgs.fetchgit {
|
||||||
|
url = "https://gerrit.googlesource.com/plugins/code-owners";
|
||||||
|
rev = "6fdf3ce2e52904b35e2a5824a4197155c2c6b4e4";
|
||||||
|
sha256 = "sha256:17k6310py71wax3881mf3vsf9zas648j4xzs9h0d7migv5nzsdzs";
|
||||||
|
};
|
||||||
|
patches = [
|
||||||
|
./using-usernames.patch
|
||||||
|
];
|
||||||
|
}
|
||||||
425
third_party/gerrit_plugins/code-owners/using-usernames.patch
vendored
Normal file
425
third_party/gerrit_plugins/code-owners/using-usernames.patch
vendored
Normal file
|
|
@ -0,0 +1,425 @@
|
||||||
|
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
|
||||||
|
index 07894ced..6a528b50 100644
|
||||||
|
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
|
||||||
|
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
|
||||||
|
@@ -17,6 +17,8 @@ package com.google.gerrit.plugins.codeowners.backend;
|
||||||
|
import static com.google.common.base.Preconditions.checkState;
|
||||||
|
import static com.google.common.collect.ImmutableMap.toImmutableMap;
|
||||||
|
import static com.google.common.collect.ImmutableSet.toImmutableSet;
|
||||||
|
+import static com.google.common.collect.ImmutableSetMultimap.flatteningToImmutableSetMultimap;
|
||||||
|
+import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
|
||||||
|
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
|
||||||
|
import static java.util.Objects.requireNonNull;
|
||||||
|
|
||||||
|
@@ -25,6 +27,7 @@ import com.google.common.collect.ImmutableList;
|
||||||
|
import com.google.common.collect.ImmutableMap;
|
||||||
|
import com.google.common.collect.ImmutableMultimap;
|
||||||
|
import com.google.common.collect.ImmutableSet;
|
||||||
|
+import com.google.common.collect.ImmutableSetMultimap;
|
||||||
|
import com.google.common.collect.Iterables;
|
||||||
|
import com.google.common.collect.Streams;
|
||||||
|
import com.google.common.flogger.FluentLogger;
|
||||||
|
@@ -38,6 +41,9 @@ import com.google.gerrit.server.IdentifiedUser;
|
||||||
|
import com.google.gerrit.server.account.AccountCache;
|
||||||
|
import com.google.gerrit.server.account.AccountControl;
|
||||||
|
import com.google.gerrit.server.account.AccountState;
|
||||||
|
+import com.google.gerrit.server.account.GroupBackend;
|
||||||
|
+import com.google.gerrit.server.account.GroupBackends;
|
||||||
|
+import com.google.gerrit.server.account.InternalGroupBackend;
|
||||||
|
import com.google.gerrit.server.account.externalids.ExternalId;
|
||||||
|
import com.google.gerrit.server.account.externalids.ExternalIds;
|
||||||
|
import com.google.gerrit.server.permissions.GlobalPermission;
|
||||||
|
@@ -103,6 +109,8 @@ public class CodeOwnerResolver {
|
||||||
|
|
||||||
|
@VisibleForTesting public static final String ALL_USERS_WILDCARD = "*";
|
||||||
|
|
||||||
|
+ public static final String GROUP_PREFIX = "group:";
|
||||||
|
+
|
||||||
|
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
|
||||||
|
private final PermissionBackend permissionBackend;
|
||||||
|
private final Provider<CurrentUser> currentUser;
|
||||||
|
@@ -113,6 +121,7 @@ public class CodeOwnerResolver {
|
||||||
|
private final CodeOwnerMetrics codeOwnerMetrics;
|
||||||
|
private final UnresolvedImportFormatter unresolvedImportFormatter;
|
||||||
|
private final TransientCodeOwnerCache transientCodeOwnerCache;
|
||||||
|
+ private final InternalGroupBackend groupBackend;
|
||||||
|
|
||||||
|
// Enforce visibility by default.
|
||||||
|
private boolean enforceVisibility = true;
|
||||||
|
@@ -133,7 +142,8 @@ public class CodeOwnerResolver {
|
||||||
|
PathCodeOwners.Factory pathCodeOwnersFactory,
|
||||||
|
CodeOwnerMetrics codeOwnerMetrics,
|
||||||
|
UnresolvedImportFormatter unresolvedImportFormatter,
|
||||||
|
- TransientCodeOwnerCache transientCodeOwnerCache) {
|
||||||
|
+ TransientCodeOwnerCache transientCodeOwnerCache,
|
||||||
|
+ InternalGroupBackend groupBackend) {
|
||||||
|
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
|
||||||
|
this.permissionBackend = permissionBackend;
|
||||||
|
this.currentUser = currentUser;
|
||||||
|
@@ -144,6 +154,7 @@ public class CodeOwnerResolver {
|
||||||
|
this.codeOwnerMetrics = codeOwnerMetrics;
|
||||||
|
this.unresolvedImportFormatter = unresolvedImportFormatter;
|
||||||
|
this.transientCodeOwnerCache = transientCodeOwnerCache;
|
||||||
|
+ this.groupBackend = groupBackend;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
@@ -357,6 +368,12 @@ public class CodeOwnerResolver {
|
||||||
|
"cannot resolve code owner email %s: no account with this email exists",
|
||||||
|
CodeOwnerResolver.ALL_USERS_WILDCARD));
|
||||||
|
}
|
||||||
|
+ if (codeOwnerReference.email().startsWith(GROUP_PREFIX)) {
|
||||||
|
+ return OptionalResultWithMessages.createEmpty(
|
||||||
|
+ String.format(
|
||||||
|
+ "cannot resolve code owner email %s: this is a group",
|
||||||
|
+ codeOwnerReference.email()));
|
||||||
|
+ }
|
||||||
|
|
||||||
|
ImmutableList.Builder<String> messageBuilder = ImmutableList.builder();
|
||||||
|
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
|
||||||
|
@@ -401,9 +418,32 @@ public class CodeOwnerResolver {
|
||||||
|
ImmutableMultimap<CodeOwnerReference, CodeOwnerAnnotation> annotations) {
|
||||||
|
requireNonNull(codeOwnerReferences, "codeOwnerReferences");
|
||||||
|
|
||||||
|
+ ImmutableSet<String> groupsToResolve =
|
||||||
|
+ codeOwnerReferences.stream()
|
||||||
|
+ .map(CodeOwnerReference::email)
|
||||||
|
+ .filter(ref -> ref.startsWith(GROUP_PREFIX))
|
||||||
|
+ .map(ref -> ref.substring(GROUP_PREFIX.length()))
|
||||||
|
+ .collect(toImmutableSet());
|
||||||
|
+
|
||||||
|
+ ImmutableSetMultimap<String, CodeOwner> resolvedGroups =
|
||||||
|
+ groupsToResolve.stream()
|
||||||
|
+ .map(groupName -> GroupBackends.findExactSuggestion(groupBackend, groupName))
|
||||||
|
+ .filter(groupRef -> groupRef != null)
|
||||||
|
+ .filter(groupRef -> groupBackend.isVisibleToAll(groupRef.getUUID()))
|
||||||
|
+ .map(groupRef -> groupBackend.get(groupRef.getUUID()))
|
||||||
|
+ .collect(flatteningToImmutableSetMultimap(
|
||||||
|
+ groupRef -> GROUP_PREFIX + groupRef.getName(),
|
||||||
|
+ groupRef -> accountCache
|
||||||
|
+ .get(ImmutableSet.copyOf(groupRef.getMembers()))
|
||||||
|
+ .values().stream()
|
||||||
|
+ .map(accountState -> CodeOwner.create(accountState.account().id()))));
|
||||||
|
+ ImmutableSetMultimap<CodeOwner, String> usersToGroups =
|
||||||
|
+ resolvedGroups.inverse();
|
||||||
|
+
|
||||||
|
ImmutableSet<String> emailsToResolve =
|
||||||
|
codeOwnerReferences.stream()
|
||||||
|
.map(CodeOwnerReference::email)
|
||||||
|
+ .filter(ref -> !ref.startsWith(GROUP_PREFIX))
|
||||||
|
.filter(filterOutAllUsersWildCard(ownedByAllUsers))
|
||||||
|
.collect(toImmutableSet());
|
||||||
|
|
||||||
|
@@ -438,7 +478,8 @@ public class CodeOwnerResolver {
|
||||||
|
ImmutableMap<String, CodeOwner> codeOwnersByEmail =
|
||||||
|
accountsByEmail.map(mapToCodeOwner()).collect(toImmutableMap(Pair::key, Pair::value));
|
||||||
|
|
||||||
|
- if (codeOwnersByEmail.keySet().size() < emailsToResolve.size()) {
|
||||||
|
+ if (codeOwnersByEmail.keySet().size() < emailsToResolve.size() ||
|
||||||
|
+ resolvedGroups.keySet().size() < groupsToResolve.size()) {
|
||||||
|
hasUnresolvedCodeOwners.set(true);
|
||||||
|
}
|
||||||
|
|
||||||
|
@@ -452,7 +493,9 @@ public class CodeOwnerResolver {
|
||||||
|
cachedCodeOwnersByEmail.entrySet().stream()
|
||||||
|
.filter(e -> e.getValue().isPresent())
|
||||||
|
.map(e -> Pair.of(e.getKey(), e.getValue().get()));
|
||||||
|
- Streams.concat(newlyResolvedCodeOwnersStream, cachedCodeOwnersStream)
|
||||||
|
+ Stream<Pair<String, CodeOwner>> resolvedGroupsCodeOwnersStream =
|
||||||
|
+ resolvedGroups.entries().stream().map(e -> Pair.of(e.getKey(), e.getValue()));
|
||||||
|
+ Streams.concat(Streams.concat(newlyResolvedCodeOwnersStream, cachedCodeOwnersStream), resolvedGroupsCodeOwnersStream)
|
||||||
|
.forEach(
|
||||||
|
p -> {
|
||||||
|
ImmutableSet.Builder<CodeOwnerAnnotation> annotationBuilder = ImmutableSet.builder();
|
||||||
|
@@ -463,6 +506,12 @@ public class CodeOwnerResolver {
|
||||||
|
annotationBuilder.addAll(
|
||||||
|
annotations.get(CodeOwnerReference.create(ALL_USERS_WILDCARD)));
|
||||||
|
|
||||||
|
+ // annotations for the groups this user is in apply as well
|
||||||
|
+ for (String group : usersToGroups.get(p.value())) {
|
||||||
|
+ annotationBuilder.addAll(
|
||||||
|
+ annotations.get(CodeOwnerReference.create(group)));
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
if (!codeOwnersWithAnnotations.containsKey(p.value())) {
|
||||||
|
codeOwnersWithAnnotations.put(p.value(), new HashSet<>());
|
||||||
|
}
|
||||||
|
@@ -566,7 +615,7 @@ public class CodeOwnerResolver {
|
||||||
|
}
|
||||||
|
|
||||||
|
messages.add(String.format("email %s has no domain", email));
|
||||||
|
- return false;
|
||||||
|
+ return true; // TVL: we allow domain-less strings which are treated as usernames.
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
@@ -581,11 +630,29 @@ public class CodeOwnerResolver {
|
||||||
|
*/
|
||||||
|
private ImmutableMap<String, Collection<ExternalId>> lookupExternalIds(
|
||||||
|
ImmutableList.Builder<String> messages, ImmutableSet<String> emails) {
|
||||||
|
+ String[] actualEmails = emails.stream()
|
||||||
|
+ .filter(email -> email.contains("@"))
|
||||||
|
+ .toArray(String[]::new);
|
||||||
|
+ ImmutableSet<String> usernames = emails.stream()
|
||||||
|
+ .filter(email -> !email.contains("@"))
|
||||||
|
+ .collect(ImmutableSet.toImmutableSet());
|
||||||
|
try {
|
||||||
|
- ImmutableMap<String, Collection<ExternalId>> extIdsByEmail =
|
||||||
|
- externalIds.byEmails(emails.toArray(new String[0])).asMap();
|
||||||
|
+ ImmutableMap<String, Collection<ExternalId>> extIds =
|
||||||
|
+ new ImmutableMap.Builder<String, Collection<ExternalId>>()
|
||||||
|
+ .putAll(externalIds.byEmails(actualEmails).asMap())
|
||||||
|
+ .putAll(externalIds.allByAccount().entries().stream()
|
||||||
|
+ .map(entry -> entry.getValue())
|
||||||
|
+ .filter(externalId ->
|
||||||
|
+ externalId.key().scheme() != null &&
|
||||||
|
+ externalId.key().isScheme(ExternalId.SCHEME_USERNAME) &&
|
||||||
|
+ usernames.contains(externalId.key().id()))
|
||||||
|
+ .collect(toImmutableSetMultimap(
|
||||||
|
+ externalId -> externalId.key().id(),
|
||||||
|
+ externalId -> externalId))
|
||||||
|
+ .asMap())
|
||||||
|
+ .build();
|
||||||
|
emails.stream()
|
||||||
|
- .filter(email -> !extIdsByEmail.containsKey(email))
|
||||||
|
+ .filter(email -> !extIds.containsKey(email))
|
||||||
|
.forEach(
|
||||||
|
email -> {
|
||||||
|
transientCodeOwnerCache.cacheNonResolvable(email);
|
||||||
|
@@ -594,7 +661,7 @@ public class CodeOwnerResolver {
|
||||||
|
"cannot resolve code owner email %s: no account with this email exists",
|
||||||
|
email));
|
||||||
|
});
|
||||||
|
- return extIdsByEmail;
|
||||||
|
+ return extIds;
|
||||||
|
} catch (IOException e) {
|
||||||
|
throw newInternalServerError(
|
||||||
|
String.format("cannot resolve code owner emails: %s", emails), e);
|
||||||
|
@@ -811,6 +878,15 @@ public class CodeOwnerResolver {
|
||||||
|
user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
+ if (!email.contains("@")) {
|
||||||
|
+ // the email is the username of the account, or a group, or something else.
|
||||||
|
+ messages.add(
|
||||||
|
+ String.format(
|
||||||
|
+ "account %s is visible to user %s",
|
||||||
|
+ accountState.account().id(),
|
||||||
|
+ user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
|
||||||
|
+ return true;
|
||||||
|
+ }
|
||||||
|
|
||||||
|
if (user != null) {
|
||||||
|
if (user.hasEmailAddress(email)) {
|
||||||
|
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
|
||||||
|
index 5f350998..7977ba55 100644
|
||||||
|
--- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
|
||||||
|
+++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
|
||||||
|
@@ -149,7 +149,8 @@ public class FindOwnersCodeOwnerConfigParser implements CodeOwnerConfigParser {
|
||||||
|
private static final String EOL = "[\\s]*(#.*)?$"; // end-of-line
|
||||||
|
private static final String GLOB = "[^\\s,=]+"; // a file glob
|
||||||
|
|
||||||
|
- private static final String EMAIL_OR_STAR = "([^\\s<>@,]+@[^\\s<>@#,]+|\\*)";
|
||||||
|
+ // Also allows usernames, and group:$GROUP_NAME.
|
||||||
|
+ private static final String EMAIL_OR_STAR = "([^\\s<>@,]+@[^\\s<>@#,]+?|\\*|[a-zA-Z0-9_\\-]+|group:[a-zA-Z0-9_\\-]+)";
|
||||||
|
private static final String EMAIL_LIST =
|
||||||
|
"(" + EMAIL_OR_STAR + "(" + COMMA + EMAIL_OR_STAR + ")*)";
|
||||||
|
|
||||||
|
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
|
||||||
|
index 7ec92959..59cf7e05 100644
|
||||||
|
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
|
||||||
|
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
|
||||||
|
@@ -424,7 +424,7 @@ public abstract class AbstractFileBasedCodeOwnerBackendTest extends AbstractCode
|
||||||
|
.commit()
|
||||||
|
.parent(head)
|
||||||
|
.message("Add invalid test code owner config")
|
||||||
|
- .add(JgitPath.of(codeOwnerConfigKey.filePath(getFileName())).get(), "INVALID"));
|
||||||
|
+ .add(JgitPath.of(codeOwnerConfigKey.filePath(getFileName())).get(), "INVALID!"));
|
||||||
|
}
|
||||||
|
|
||||||
|
// Try to update the code owner config.
|
||||||
|
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
|
||||||
|
index b32c3b5e..6b0f0cf8 100644
|
||||||
|
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
|
||||||
|
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
|
||||||
|
@@ -24,8 +24,10 @@ import com.google.gerrit.acceptance.TestAccount;
|
||||||
|
import com.google.gerrit.acceptance.TestMetricMaker;
|
||||||
|
import com.google.gerrit.acceptance.config.GerritConfig;
|
||||||
|
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
|
||||||
|
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
|
||||||
|
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
|
||||||
|
import com.google.gerrit.entities.Account;
|
||||||
|
+import com.google.gerrit.entities.AccountGroup;
|
||||||
|
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
|
||||||
|
import com.google.gerrit.server.ServerInitiated;
|
||||||
|
import com.google.gerrit.server.account.AccountsUpdate;
|
||||||
|
@@ -51,6 +53,7 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
|
||||||
|
@Inject private RequestScopeOperations requestScopeOperations;
|
||||||
|
@Inject @ServerInitiated private Provider<AccountsUpdate> accountsUpdate;
|
||||||
|
@Inject private AccountOperations accountOperations;
|
||||||
|
+ @Inject private GroupOperations groupOperations;
|
||||||
|
@Inject private ExternalIdNotes.Factory externalIdNotesFactory;
|
||||||
|
@Inject private TestMetricMaker testMetricMaker;
|
||||||
|
@Inject private ExternalIdFactory externalIdFactory;
|
||||||
|
@@ -112,6 +115,18 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
|
||||||
|
.contains(String.format("account %s is visible to user %s", admin.id(), admin.username()));
|
||||||
|
}
|
||||||
|
|
||||||
|
+ @Test
|
||||||
|
+ public void resolveCodeOwnerReferenceForUsername() throws Exception {
|
||||||
|
+ OptionalResultWithMessages<CodeOwner> result =
|
||||||
|
+ codeOwnerResolverProvider
|
||||||
|
+ .get()
|
||||||
|
+ .resolveWithMessages(CodeOwnerReference.create(admin.username()));
|
||||||
|
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
|
||||||
|
+ assertThat(result)
|
||||||
|
+ .hasMessagesThat()
|
||||||
|
+ .contains(String.format("account %s is visible to user %s", admin.id(), admin.username()));
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
@Test
|
||||||
|
public void cannotResolveCodeOwnerReferenceForStarAsEmail() throws Exception {
|
||||||
|
OptionalResultWithMessages<CodeOwner> result =
|
||||||
|
@@ -127,6 +142,18 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
|
||||||
|
CodeOwnerResolver.ALL_USERS_WILDCARD));
|
||||||
|
}
|
||||||
|
|
||||||
|
+ @Test
|
||||||
|
+ public void cannotResolveCodeOwnerReferenceForGroup() throws Exception {
|
||||||
|
+ OptionalResultWithMessages<CodeOwner> result =
|
||||||
|
+ codeOwnerResolverProvider
|
||||||
|
+ .get()
|
||||||
|
+ .resolveWithMessages(CodeOwnerReference.create("group:Administrators"));
|
||||||
|
+ assertThat(result).isEmpty();
|
||||||
|
+ assertThat(result)
|
||||||
|
+ .hasMessagesThat()
|
||||||
|
+ .contains("cannot resolve code owner email group:Administrators: this is a group");
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
@Test
|
||||||
|
public void resolveCodeOwnerReferenceForAmbiguousEmailIfOtherAccountIsInactive()
|
||||||
|
throws Exception {
|
||||||
|
@@ -391,6 +418,64 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
|
||||||
|
assertThat(result.hasUnresolvedCodeOwners()).isFalse();
|
||||||
|
}
|
||||||
|
|
||||||
|
+ @Test
|
||||||
|
+ public void resolvePathCodeOwnersWhenNonVisibleGroupIsUsed() throws Exception {
|
||||||
|
+ CodeOwnerConfig codeOwnerConfig =
|
||||||
|
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
|
||||||
|
+ .addCodeOwnerSet(
|
||||||
|
+ CodeOwnerSet.createWithoutPathExpressions("group:Administrators"))
|
||||||
|
+ .build();
|
||||||
|
+
|
||||||
|
+ CodeOwnerResolverResult result =
|
||||||
|
+ codeOwnerResolverProvider
|
||||||
|
+ .get()
|
||||||
|
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
|
||||||
|
+ assertThat(result.codeOwnersAccountIds()).isEmpty();
|
||||||
|
+ assertThat(result.ownedByAllUsers()).isFalse();
|
||||||
|
+ assertThat(result.hasUnresolvedCodeOwners()).isTrue();
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
+ @Test
|
||||||
|
+ public void resolvePathCodeOwnersWhenVisibleGroupIsUsed() throws Exception {
|
||||||
|
+ AccountGroup.UUID createdGroupUUID = groupOperations
|
||||||
|
+ .newGroup()
|
||||||
|
+ .name("VisibleGroup")
|
||||||
|
+ .visibleToAll(true)
|
||||||
|
+ .addMember(admin.id())
|
||||||
|
+ .create();
|
||||||
|
+
|
||||||
|
+ CodeOwnerConfig codeOwnerConfig =
|
||||||
|
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
|
||||||
|
+ .addCodeOwnerSet(
|
||||||
|
+ CodeOwnerSet.createWithoutPathExpressions("group:VisibleGroup"))
|
||||||
|
+ .build();
|
||||||
|
+
|
||||||
|
+ CodeOwnerResolverResult result =
|
||||||
|
+ codeOwnerResolverProvider
|
||||||
|
+ .get()
|
||||||
|
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
|
||||||
|
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id());
|
||||||
|
+ assertThat(result.ownedByAllUsers()).isFalse();
|
||||||
|
+ assertThat(result.hasUnresolvedCodeOwners()).isFalse();
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
+ @Test
|
||||||
|
+ public void resolvePathCodeOwnersWhenUsernameIsUsed() throws Exception {
|
||||||
|
+ CodeOwnerConfig codeOwnerConfig =
|
||||||
|
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
|
||||||
|
+ .addCodeOwnerSet(
|
||||||
|
+ CodeOwnerSet.createWithoutPathExpressions(admin.username()))
|
||||||
|
+ .build();
|
||||||
|
+
|
||||||
|
+ CodeOwnerResolverResult result =
|
||||||
|
+ codeOwnerResolverProvider
|
||||||
|
+ .get()
|
||||||
|
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
|
||||||
|
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id());
|
||||||
|
+ assertThat(result.ownedByAllUsers()).isFalse();
|
||||||
|
+ assertThat(result.hasUnresolvedCodeOwners()).isFalse();
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
@Test
|
||||||
|
public void resolvePathCodeOwnersNonResolvableCodeOwnersAreFilteredOut() throws Exception {
|
||||||
|
CodeOwnerConfig codeOwnerConfig =
|
||||||
|
@@ -649,7 +734,7 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
|
||||||
|
"domain example.com of email foo@example.org@example.com is allowed");
|
||||||
|
assertIsEmailDomainAllowed(
|
||||||
|
"foo@example.org", false, "domain example.org of email foo@example.org is not allowed");
|
||||||
|
- assertIsEmailDomainAllowed("foo", false, "email foo has no domain");
|
||||||
|
+ assertIsEmailDomainAllowed("foo", true, "email foo has no domain");
|
||||||
|
assertIsEmailDomainAllowed(
|
||||||
|
"foo@example.com@example.org",
|
||||||
|
false,
|
||||||
|
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
|
||||||
|
index 260e635e..7aab99d0 100644
|
||||||
|
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
|
||||||
|
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
|
||||||
|
@@ -158,16 +158,42 @@ public class FindOwnersCodeOwnerConfigParserTest extends AbstractCodeOwnerConfig
|
||||||
|
codeOwnerConfigParser.parse(
|
||||||
|
TEST_REVISION,
|
||||||
|
CodeOwnerConfig.Key.create(project, "master", "/"),
|
||||||
|
- getCodeOwnerConfig(EMAIL_1, "INVALID", "NOT_AN_EMAIL", EMAIL_2)));
|
||||||
|
+ getCodeOwnerConfig(EMAIL_1, "INVALID!", "NOT!AN_EMAIL", EMAIL_2)));
|
||||||
|
assertThat(exception.getFullMessage(FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME))
|
||||||
|
.isEqualTo(
|
||||||
|
String.format(
|
||||||
|
"invalid code owner config file '/OWNERS' (project = %s, branch = master):\n"
|
||||||
|
- + " invalid line: INVALID\n"
|
||||||
|
- + " invalid line: NOT_AN_EMAIL",
|
||||||
|
+ + " invalid line: INVALID!\n"
|
||||||
|
+ + " invalid line: NOT!AN_EMAIL",
|
||||||
|
project));
|
||||||
|
}
|
||||||
|
|
||||||
|
+ @Test
|
||||||
|
+ public void codeOwnerConfigWithUsernames() throws Exception {
|
||||||
|
+ assertParseAndFormat(
|
||||||
|
+ getCodeOwnerConfig(EMAIL_1, "USERNAME", EMAIL_2),
|
||||||
|
+ codeOwnerConfig ->
|
||||||
|
+ assertThat(codeOwnerConfig)
|
||||||
|
+ .hasCodeOwnerSetsThat()
|
||||||
|
+ .onlyElement()
|
||||||
|
+ .hasCodeOwnersEmailsThat()
|
||||||
|
+ .containsExactly(EMAIL_1, "USERNAME", EMAIL_2),
|
||||||
|
+ getCodeOwnerConfig(EMAIL_1, "USERNAME", EMAIL_2));
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
+ @Test
|
||||||
|
+ public void codeOwnerConfigWithGroups() throws Exception {
|
||||||
|
+ assertParseAndFormat(
|
||||||
|
+ getCodeOwnerConfig(EMAIL_1, "group:tvl-employees", EMAIL_2),
|
||||||
|
+ codeOwnerConfig ->
|
||||||
|
+ assertThat(codeOwnerConfig)
|
||||||
|
+ .hasCodeOwnerSetsThat()
|
||||||
|
+ .onlyElement()
|
||||||
|
+ .hasCodeOwnersEmailsThat()
|
||||||
|
+ .containsExactly(EMAIL_1, "group:tvl-employees", EMAIL_2),
|
||||||
|
+ getCodeOwnerConfig(EMAIL_1, "group:tvl-employees", EMAIL_2));
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
@Test
|
||||||
|
public void codeOwnerConfigWithComment() throws Exception {
|
||||||
|
assertParseAndFormat(
|
||||||
Loading…
Reference in a new issue