Merge changes I9bc8a28d,I4352392a,Ic685a93d
* changes:
CodeOwnerApprovalCheck: Do not check twice if a global owner is reviewer
CodeOwnerApprovalCheck: Drop unreachable code
Simplify assertions in CodeOwnerApprovalCheck*Test
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
index 4e381d7..412aebc 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
@@ -102,6 +102,11 @@
assumeThatCodeOwnersBackendIsNotProtoBackend();
}
+ protected void skipTestIfAnnotationsNotSupportedByCodeOwnersBackend() {
+ // the proto backend doesn't support annotations on code owners
+ assumeThatCodeOwnersBackendIsNotProtoBackend();
+ }
+
protected void assumeThatCodeOwnersBackendIsNotProtoBackend() {
assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
index c96645c..19b02a0 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
@@ -131,6 +131,16 @@
/** Whether the the specified path in the branch is owned by all users (aka {@code *}). */
public boolean isOwnedByAllUsers;
+ /**
+ * Annotations that were set for the user.
+ *
+ * <p>Contains only supported annotations (unsupported annotations are reported in the {@link
+ * #debugLogs}).
+ *
+ * <p>Sorted alphabetically.
+ */
+ public List<String> annotations;
+
/** Debug logs that may help to understand why the user is or isn't a code owner. */
public List<String> debugLogs;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerAnnotation.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerAnnotation.java
new file mode 100644
index 0000000..bcb4d8c
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerAnnotation.java
@@ -0,0 +1,31 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import com.google.auto.value.AutoValue;
+
+/**
+ * An annotation on a {@link CodeOwnerReference} in a {@link CodeOwnerConfig}.
+ *
+ * <p>This is a class rather than string so that we can easily support values on annotations later.
+ */
+@AutoValue
+public abstract class CodeOwnerAnnotation {
+ public abstract String key();
+
+ public static CodeOwnerAnnotation create(String key) {
+ return new AutoValue_CodeOwnerAnnotation(key);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerAnnotations.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerAnnotations.java
new file mode 100644
index 0000000..a9d3edb
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerAnnotations.java
@@ -0,0 +1,47 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/** Class that defines all known/supported {@link CodeOwnerAnnotation}s on code owners. */
+public class CodeOwnerAnnotations {
+ /**
+ * Code owners with this annotation are omitted when suggesting code owners (see {@link
+ * com.google.gerrit.plugins.codeowners.restapi.GetCodeOwnersForPathInChange}).
+ */
+ public static final CodeOwnerAnnotation NEVER_SUGGEST_ANNOTATION =
+ CodeOwnerAnnotation.create("NEVER_SUGGEST");
+
+ private static final List<String> KEYS_ALL;
+
+ static {
+ KEYS_ALL = new ArrayList<>();
+ KEYS_ALL.add(NEVER_SUGGEST_ANNOTATION.key());
+ }
+
+ /** Whether the given annotation is known and supported. */
+ public static boolean isSupported(String annotation) {
+ return KEYS_ALL.contains(annotation);
+ }
+
+ /**
+ * Private constructor to prevent instantiation of this class.
+ *
+ * <p>This class contains only static method and hence never needs to be instantiated.
+ */
+ private CodeOwnerAnnotations() {}
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index b363dd2..6ed577e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -22,8 +22,10 @@
import com.google.common.annotations.VisibleForTesting;
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.Iterables;
+import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Project;
@@ -228,6 +230,7 @@
pathCodeOwners.resolveCodeOwnerConfig();
return resolve(
pathCodeOwnersResult.get().getPathCodeOwners(),
+ pathCodeOwnersResult.get().getAnnotations(),
pathCodeOwnersResult.get().unresolvedImports(),
pathCodeOwnersResult.messages());
}
@@ -253,6 +256,7 @@
public CodeOwnerResolverResult resolve(Set<CodeOwnerReference> codeOwnerReferences) {
return resolve(
codeOwnerReferences,
+ /* annotations= */ ImmutableMultimap.of(),
/* unresolvedImports= */ ImmutableList.of(),
/* pathCodeOwnersMessages= */ ImmutableList.of());
}
@@ -264,12 +268,14 @@
* parallel (via {@link AccountCache#get(Set)}.
*
* @param codeOwnerReferences the code owner references that should be resolved
+ * @param annotationsByCodeOwnerReference annotations by code owner reference
* @param unresolvedImports list of unresolved imports
* @param pathCodeOwnersMessages messages that were collected when resolving path code owners
* @return the resolved code owner references as a {@link CodeOwnerResolverResult}
*/
private CodeOwnerResolverResult resolve(
Set<CodeOwnerReference> codeOwnerReferences,
+ ImmutableMultimap<CodeOwnerReference, CodeOwnerAnnotation> annotationsByCodeOwnerReference,
List<UnresolvedImport> unresolvedImports,
ImmutableList<String> pathCodeOwnersMessages) {
requireNonNull(codeOwnerReferences, "codeOwnerReferences");
@@ -285,12 +291,23 @@
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
AtomicBoolean hasUnresolvedCodeOwners = new AtomicBoolean(false);
- ImmutableSet<CodeOwner> codeOwners =
- resolve(messageBuilder, ownedByAllUsers, hasUnresolvedCodeOwners, codeOwnerReferences);
+ ImmutableMap<CodeOwner, ImmutableSet<CodeOwnerAnnotation>> codeOwnersWithAnnotations =
+ resolve(
+ messageBuilder,
+ ownedByAllUsers,
+ hasUnresolvedCodeOwners,
+ codeOwnerReferences,
+ annotationsByCodeOwnerReference);
+
+ ImmutableMultimap.Builder<CodeOwner, CodeOwnerAnnotation> annotationsByCodeOwner =
+ ImmutableMultimap.builder();
+ codeOwnersWithAnnotations.forEach(
+ (codeOwner, annotations) -> annotationsByCodeOwner.putAll(codeOwner, annotations));
CodeOwnerResolverResult codeOwnerResolverResult =
CodeOwnerResolverResult.create(
- codeOwners,
+ codeOwnersWithAnnotations.keySet(),
+ annotationsByCodeOwner.build(),
ownedByAllUsers.get(),
hasUnresolvedCodeOwners.get(),
!unresolvedImports.isEmpty(),
@@ -341,17 +358,19 @@
ImmutableList.Builder<String> messageBuilder = ImmutableList.builder();
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
AtomicBoolean hasUnresolvedCodeOwners = new AtomicBoolean(false);
- ImmutableSet<CodeOwner> codeOwners =
+ ImmutableMap<CodeOwner, ImmutableSet<CodeOwnerAnnotation>> codeOwnersWithAnnotations =
resolve(
messageBuilder,
ownedByAllUsers,
hasUnresolvedCodeOwners,
- ImmutableSet.of(codeOwnerReference));
+ ImmutableSet.of(codeOwnerReference),
+ /* annotations= */ ImmutableMultimap.of());
ImmutableList<String> messages = messageBuilder.build();
- if (codeOwners.isEmpty()) {
+ if (codeOwnersWithAnnotations.isEmpty()) {
return OptionalResultWithMessages.createEmpty(messages);
}
- return OptionalResultWithMessages.create(Iterables.getOnlyElement(codeOwners), messages);
+ return OptionalResultWithMessages.create(
+ Iterables.getOnlyElement(codeOwnersWithAnnotations.keySet()), messages);
}
/**
@@ -366,13 +385,17 @@
* @param hasUnresolvedCodeOwners a flag that is set any of the given {@link CodeOwnerReference}s
* cannot be resolved
* @param codeOwnerReferences the code owner references that should be resolved
- * @return the resolved code owner references as a {@link CodeOwner}s
+ * @param annotations annotations by code owner reference
+ * @return map that maps the resolved {@link CodeOwner}s to their annotations (note: we cannot
+ * return a {@code Multimap<CodeOwner, CodeOwnerAnnotation>} here since there may be code
+ * owners without annotations and Multimap doesn't store keys for which no values are stored)
*/
- private ImmutableSet<CodeOwner> resolve(
+ private ImmutableMap<CodeOwner, ImmutableSet<CodeOwnerAnnotation>> resolve(
ImmutableList.Builder<String> messages,
AtomicBoolean ownedByAllUsers,
AtomicBoolean hasUnresolvedCodeOwners,
- Set<CodeOwnerReference> codeOwnerReferences) {
+ Set<CodeOwnerReference> codeOwnerReferences,
+ ImmutableMultimap<CodeOwnerReference, CodeOwnerAnnotation> annotations) {
requireNonNull(codeOwnerReferences, "codeOwnerReferences");
ImmutableSet<String> emailsToResolve =
@@ -416,16 +439,30 @@
hasUnresolvedCodeOwners.set(true);
}
- ImmutableSet<CodeOwner> cachedCodeOwners =
- cachedCodeOwnersByEmail.values().stream()
- .filter(Optional::isPresent)
- .map(Optional::get)
- .collect(toImmutableSet());
+ // Merge code owners that have been newly resolved with code owners which have been looked up
+ // from cache and return them with their annotations.
+ Stream<Pair<String, CodeOwner>> newlyResolvedCodeOwnersStream =
+ codeOwnersByEmail.entrySet().stream().map(e -> Pair.of(e.getKey(), e.getValue()));
+ Stream<Pair<String, CodeOwner>> cachedCodeOwnersStream =
+ cachedCodeOwnersByEmail.entrySet().stream()
+ .filter(e -> e.getValue().isPresent())
+ .map(e -> Pair.of(e.getKey(), e.getValue().get()));
+ return Streams.concat(newlyResolvedCodeOwnersStream, cachedCodeOwnersStream)
+ .collect(
+ toImmutableMap(
+ Pair::value,
+ p -> {
+ ImmutableSet.Builder<CodeOwnerAnnotation> annotationBuilder =
+ ImmutableSet.builder();
- ImmutableSet.Builder<CodeOwner> codeOwners = ImmutableSet.builder();
- codeOwners.addAll(cachedCodeOwners);
- codeOwners.addAll(codeOwnersByEmail.values());
- return codeOwners.build();
+ annotationBuilder.addAll(annotations.get(CodeOwnerReference.create(p.key())));
+
+ // annotations for the all users wildcard (aka '*') apply to all code owners
+ annotationBuilder.addAll(
+ annotations.get(CodeOwnerReference.create(ALL_USERS_WILDCARD)));
+
+ return annotationBuilder.build();
+ }));
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
index 0744e5b..7dd9e96 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
@@ -19,6 +19,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Account;
import java.util.List;
@@ -39,6 +40,9 @@
return codeOwners().stream().map(CodeOwner::accountId).collect(toImmutableSet());
}
+ /** Returns the annotations for the {@link #codeOwners()}. */
+ public abstract ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations();
+
/**
* Whether the code ownership was assigned to all users by using the {@link
* CodeOwnerResolver#ALL_USERS_WILDCARD}.
@@ -69,6 +73,7 @@
public final String toString() {
return MoreObjects.toStringHelper(this)
.add("codeOwners", codeOwners())
+ .add("annotations", annotations())
.add("ownedByAllUsers", ownedByAllUsers())
.add("hasUnresolvedCodeOwners", hasUnresolvedCodeOwners())
.add("hasUnresolvedImports", hasUnresolvedImports())
@@ -79,12 +84,14 @@
/** Creates a {@link CodeOwnerResolverResult} instance. */
public static CodeOwnerResolverResult create(
ImmutableSet<CodeOwner> codeOwners,
+ ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
boolean ownedByAllUsers,
boolean hasUnresolvedCodeOwners,
boolean hasUnresolvedImports,
List<String> messages) {
return new AutoValue_CodeOwnerResolverResult(
codeOwners,
+ annotations,
ownedByAllUsers,
hasUnresolvedCodeOwners,
hasUnresolvedImports,
@@ -95,6 +102,7 @@
public static CodeOwnerResolverResult createEmpty() {
return new AutoValue_CodeOwnerResolverResult(
/* codeOwners= */ ImmutableSet.of(),
+ /* annotations= */ ImmutableMultimap.of(),
/* ownedByAllUsers= */ false,
/* hasUnresolvedCodeOwners= */ false,
/* hasUnresolvedImports= */ false,
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScore.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScore.java
index 523c2ee..40a4a3b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScore.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScore.java
@@ -53,7 +53,21 @@
* <p>The IS_REVIEWER score has a higher weight than the {@link #DISTANCE} score so that it takes
* precedence and code owners that are reviewers are always returned first.
*/
- IS_REVIEWER(Kind.GREATER_VALUE_IS_BETTER, /* weight= */ 2, /* maxValue= */ 1);
+ IS_REVIEWER(Kind.GREATER_VALUE_IS_BETTER, /* weight= */ 2, /* maxValue= */ 1),
+
+ /**
+ * Score to take into account when a user is explicitly mentioned as a code owner
+ *
+ * <p>Users that are explicitly mentioned as code owner in a code owner config file get scored
+ * with 1 (see {@link #IS_EXPLICITLY_MENTIONED_SCORING_VALUE}), while users that are not
+ * explicitly mentioned as code owners in the code owner config file, and are only code owners
+ * because the code ownership is assigned to all users aka {@code *}, get scored with 0 (see
+ * {@link #NOT_EXPLICITLY_MENTIONED_SCORING_VALUE}).
+ *
+ * <p>The IS_EXPLICITLY_MENTIONED score has a lower weight than the {@link #DISTANCE} score so
+ * that the {@link #DISTANCE} score takes precedence.
+ */
+ IS_EXPLICITLY_MENTIONED(Kind.GREATER_VALUE_IS_BETTER, /* weight= */ 0.5, /* maxValue= */ 1);
/**
* Scoring value for the {@link #IS_REVIEWER} score for users that are not a reviewer of the
@@ -67,6 +81,19 @@
public static int IS_REVIEWER_SCORING_VALUE = 1;
/**
+ * Scoring value for the {@link #IS_EXPLICITLY_MENTIONED} score for users that are not explicitly
+ * mentioned as code owners in the code owner config file and are only code owners because the
+ * code ownership is assigned to all users aka {@code *}.
+ */
+ public static int NOT_EXPLICITLY_MENTIONED_SCORING_VALUE = 0;
+
+ /**
+ * Scoring value for the {@link #IS_EXPLICITLY_MENTIONED} score for users that are explicitly
+ * mentioned as code owners in the code owner config file.
+ */
+ public static int IS_EXPLICITLY_MENTIONED_SCORING_VALUE = 1;
+
+ /**
* Score kind.
*
* <p>Whether a greater value as scoring is better than a lower value ({@code
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSet.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSet.java
index 02b6524..43f49d5 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSet.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSet.java
@@ -19,8 +19,10 @@
import static java.util.Objects.requireNonNull;
import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import java.util.Arrays;
+import java.util.Set;
/**
* A code owner set defines a set of code owners for a set of path expressions.
@@ -71,6 +73,9 @@
/** Gets the code owners of this code owner set. */
public abstract ImmutableSet<CodeOwnerReference> codeOwners();
+ /** Gets the annotations of the {@link #codeOwners()}. */
+ public abstract ImmutableMultimap<CodeOwnerReference, CodeOwnerAnnotation> annotations();
+
/**
* Creates a builder from this code owner set.
*
@@ -199,6 +204,37 @@
return this;
}
+ /** Gets a builder to add code owner annotations. */
+ abstract ImmutableMultimap.Builder<CodeOwnerReference, CodeOwnerAnnotation>
+ annotationsBuilder();
+
+ /**
+ * Adds an annotation for a code owner.
+ *
+ * @param email email of the code owner for which the annotation should be added
+ * @param annotation annotation that should be added
+ * @return the Builder instance for chaining calls
+ */
+ public Builder addAnnotation(String email, CodeOwnerAnnotation annotation) {
+ return addAnnotations(CodeOwnerReference.create(email), ImmutableSet.of(annotation));
+ }
+
+ /**
+ * Adds annotations for a code owner.
+ *
+ * @param codeOwnerReference reference to the code owner for which the annotations should be
+ * added
+ * @param annotations annotations that should be added
+ * @return the Builder instance for chaining calls
+ */
+ public Builder addAnnotations(
+ CodeOwnerReference codeOwnerReference, Set<CodeOwnerAnnotation> annotations) {
+ requireNonNull(codeOwnerReference, "codeOwnerReference");
+ requireNonNull(annotations, "annotations");
+ annotationsBuilder().putAll(codeOwnerReference, annotations);
+ return this;
+ }
+
/**
* Adds a code owner for the given email.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
index bc45ded..93a36be 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
@@ -19,6 +19,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import java.nio.file.Path;
@@ -52,7 +53,7 @@
*/
public ImmutableSet<CodeOwnerReference> getPathCodeOwners() {
logger.atFine().log(
- "computing path code owners for %s from %s", path(), codeOwnerConfig().key());
+ "retrieving path code owners for %s from %s", path(), codeOwnerConfig().key());
ImmutableSet<CodeOwnerReference> pathCodeOwners =
codeOwnerConfig().codeOwnerSets().stream()
.flatMap(codeOwnerSet -> codeOwnerSet.codeOwners().stream())
@@ -62,6 +63,34 @@
}
/**
+ * Gets the annotations for all path code owners that are returned by {@link
+ * #getPathCodeOwners()}.
+ *
+ * @return annotations by code owner
+ */
+ public ImmutableMultimap<CodeOwnerReference, CodeOwnerAnnotation> getAnnotations() {
+ logger.atFine().log(
+ "retrieving path code owner annotations for %s from %s", path(), codeOwnerConfig().key());
+ ImmutableMultimap.Builder<CodeOwnerReference, CodeOwnerAnnotation> annotationsBuilder =
+ ImmutableMultimap.builder();
+ codeOwnerConfig()
+ .codeOwnerSets()
+ .forEach(codeOwnerSet -> annotationsBuilder.putAll(codeOwnerSet.annotations()));
+
+ ImmutableMultimap<CodeOwnerReference, CodeOwnerAnnotation> annotations =
+ annotationsBuilder.build();
+ logger.atFine().log("annotations = %s", annotations);
+ return annotations;
+ }
+
+ /** Gets the annotations for the given email. */
+ public ImmutableSet<String> getAnnotationsFor(String email) {
+ return getAnnotations().get(CodeOwnerReference.create(email)).stream()
+ .map(CodeOwnerAnnotation::key)
+ .collect(toImmutableSet());
+ }
+
+ /**
* Whether parent code owners should be ignored for the path.
*
* @return whether parent code owners should be ignored for the path
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 788e921..e94875a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
@@ -15,7 +15,10 @@
package com.google.gerrit.plugins.codeowners.backend.findowners;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
+import static java.util.Comparator.naturalOrder;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.joining;
@@ -25,7 +28,12 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.MultimapBuilder;
+import com.google.common.collect.SortedSetMultimap;
+import com.google.common.collect.TreeMultimap;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotation;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigParseException;
@@ -37,7 +45,11 @@
import com.google.inject.Singleton;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
import java.util.List;
+import java.util.Set;
+import java.util.SortedSet;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Stream;
@@ -161,6 +173,7 @@
// Simple input lines with 0 or 1 sub-pattern.
private static final Pattern PAT_COMMENT = Pattern.compile(BOL + EOL);
private static final Pattern PAT_EMAIL = Pattern.compile(BOL + EMAIL_OR_STAR + EOL);
+ private static final Pattern PAT_ANNOTATION = Pattern.compile("#\\{([A-Za-z_]+)\\}");
private static final Pattern PAT_INCLUDE =
Pattern.compile(BOL + INCLUDE_OR_FILE + PROJECT_BRANCH_AND_FILE + EOL);
private static final Pattern PAT_NO_PARENT = Pattern.compile(BOL + SET_NOPARENT + EOL);
@@ -206,16 +219,18 @@
CodeOwnerSet.Builder globalCodeOwnerSetBuilder,
List<CodeOwnerSet> perFileCodeOwnerSets,
String line) {
- String email;
+ ParsedEmailLine parsedEmailLine;
CodeOwnerSet codeOwnerSet;
CodeOwnerConfigReference codeOwnerConfigReference;
if (isNoParent(line)) {
codeOwnerConfigBuilder.setIgnoreParentCodeOwners();
} else if (isComment(line)) {
// ignore comment lines and empty lines
- } else if ((email = parseEmail(line)) != null) {
- globalCodeOwnerSetBuilder.addCodeOwner(CodeOwnerReference.create(email));
- } else if ((codeOwnerSet = parsePerFile(line)) != null) {
+ } else if ((parsedEmailLine = parseEmailLine(line)) != null) {
+ globalCodeOwnerSetBuilder.addCodeOwner(parsedEmailLine.codeOwnerReference());
+ globalCodeOwnerSetBuilder.addAnnotations(
+ parsedEmailLine.codeOwnerReference(), parsedEmailLine.annotations());
+ } else if ((codeOwnerSet = parsePerFileLine(line)) != null) {
perFileCodeOwnerSets.add(codeOwnerSet);
} else if ((codeOwnerConfigReference = parseInclude(line)) != null) {
codeOwnerConfigBuilder.addImport(codeOwnerConfigReference);
@@ -224,13 +239,13 @@
}
}
- private static CodeOwnerSet parsePerFile(String line) {
- Matcher m = PAT_PER_FILE.matcher(line);
- if (!m.matches() || !isGlobs(m.group(1).trim())) {
+ private static CodeOwnerSet parsePerFileLine(String line) {
+ Matcher perFileMatcher = PAT_PER_FILE.matcher(line);
+ if (!perFileMatcher.matches() || !isGlobs(perFileMatcher.group(1).trim())) {
return null;
}
- String matchedGroup2 = m.group(2).trim();
+ String matchedGroup2 = perFileMatcher.group(2).trim();
if (!PAT_PER_FILE_OWNERS.matcher(matchedGroup2).matches()) {
checkState(
!PAT_PER_FILE_INCLUDE.matcher(matchedGroup2).matches(),
@@ -241,7 +256,9 @@
}
String[] globsAndOwners =
- new String[] {removeExtraSpaces(m.group(1)), removeExtraSpaces(m.group(2))};
+ new String[] {
+ removeExtraSpaces(perFileMatcher.group(1)), removeExtraSpaces(perFileMatcher.group(2))
+ };
String[] dirGlobs = splitGlobs(globsAndOwners[0]);
String directive = globsAndOwners[1];
if (directive.equals(TOK_SET_NOPARENT)) {
@@ -260,11 +277,27 @@
}
List<String> ownerEmails = Arrays.asList(directive.split(COMMA, -1));
- return CodeOwnerSet.builder()
- .setPathExpressions(ImmutableSet.copyOf(dirGlobs))
- .setCodeOwners(
- ownerEmails.stream().map(CodeOwnerReference::create).collect(toImmutableSet()))
- .build();
+
+ // Get the comment part of the line (the first '#' and everything that follows).
+ String comment = perFileMatcher.group(3);
+ Set<CodeOwnerAnnotation> annotations = new HashSet<>();
+ if (comment != null) {
+ Matcher annotationMatcher = PAT_ANNOTATION.matcher(comment);
+ while (annotationMatcher.find()) {
+ String annotation = annotationMatcher.group(1);
+ annotations.add(CodeOwnerAnnotation.create(annotation));
+ }
+ }
+
+ CodeOwnerSet.Builder codeOwnerSet =
+ CodeOwnerSet.builder()
+ .setPathExpressions(ImmutableSet.copyOf(dirGlobs))
+ .setCodeOwners(
+ ownerEmails.stream().map(CodeOwnerReference::create).collect(toImmutableSet()));
+ ownerEmails.stream()
+ .forEach(
+ email -> codeOwnerSet.addAnnotations(CodeOwnerReference.create(email), annotations));
+ return codeOwnerSet.build();
}
/**
@@ -327,9 +360,25 @@
return PAT_NO_PARENT.matcher(line).matches();
}
- private static String parseEmail(String line) {
- Matcher m = PAT_EMAIL.matcher(line);
- return m.matches() ? m.group(1).trim() : null;
+ private static ParsedEmailLine parseEmailLine(String line) {
+ Matcher emailMatcher = PAT_EMAIL.matcher(line);
+ if (!emailMatcher.matches()) {
+ return null;
+ }
+ String email = emailMatcher.group(1).trim();
+ ParsedEmailLine.Builder parsedEmailLine = ParsedEmailLine.builder(email);
+
+ // Get the comment part of the line (the first '#' and everything that follows).
+ String comment = emailMatcher.group(2);
+ if (comment != null) {
+ Matcher annotationMatcher = PAT_ANNOTATION.matcher(comment);
+ while (annotationMatcher.find()) {
+ String annotation = annotationMatcher.group(1);
+ parsedEmailLine.addAnnotation(annotation);
+ }
+ }
+
+ return parsedEmailLine.build();
}
private static CodeOwnerConfigReference parseInclude(String line) {
@@ -403,7 +452,7 @@
static String formatAsString(CodeOwnerConfig codeOwnerConfig) {
return formatIgnoreParentCodeOwners(codeOwnerConfig)
+ formatImports(codeOwnerConfig)
- + formatGlobalCodeOwners(codeOwnerConfig)
+ + formatFolderCodeOwners(codeOwnerConfig)
+ formatPerFileCodeOwners(codeOwnerConfig);
}
@@ -411,22 +460,37 @@
return codeOwnerConfig.ignoreParentCodeOwners() ? SET_NOPARENT_LINE : "";
}
- private static String formatGlobalCodeOwners(CodeOwnerConfig codeOwnerConfig) {
- String emails =
+ private static String formatFolderCodeOwners(CodeOwnerConfig codeOwnerConfig) {
+ ImmutableSet<CodeOwnerSet> folderCodeOwnerSets =
codeOwnerConfig.codeOwnerSets().stream()
// Filter out code owner sets with path expressions. If path expressions are present
// the code owner set defines per-file code owners and is handled in
// formatPerFileCodeOwners(CodeOwnerConfig).
.filter(codeOwnerSet -> codeOwnerSet.pathExpressions().isEmpty())
+ .collect(toImmutableSet());
+ ImmutableList<String> emails =
+ folderCodeOwnerSets.stream()
.flatMap(codeOwnerSet -> codeOwnerSet.codeOwners().stream())
.map(CodeOwnerReference::email)
.sorted()
.distinct()
- .collect(joining("\n"));
- if (!emails.isEmpty()) {
- return emails + "\n";
+ .collect(toImmutableList());
+ SortedSetMultimap<String, String> annotations = TreeMultimap.create();
+ folderCodeOwnerSets.forEach(
+ codeOwnerSet ->
+ codeOwnerSet
+ .annotations()
+ .forEach(
+ (codeOwnerReference, annotation) ->
+ annotations.put(codeOwnerReference.email(), annotation.key())));
+
+ StringBuilder b = new StringBuilder();
+ for (String email : emails) {
+ b.append(email);
+ annotations.get(email).forEach(annotation -> b.append(" #{" + annotation + "}"));
+ b.append('\n');
}
- return emails;
+ return b.toString();
}
private static String formatPerFileCodeOwners(CodeOwnerConfig codeOwnerConfig) {
@@ -458,20 +522,48 @@
}
if (!codeOwnerSet.codeOwners().isEmpty()) {
- b.append(
- String.format(
- PER_FILE_LINE_FORMAT,
- formattedPathExpressions,
- formatCodeOwnerReferencesAsList(codeOwnerSet.codeOwners())));
+ // group code owners that have the same annotations
+ ListMultimap<SortedSet<String>, CodeOwnerReference> codeOwnersByAnnotations =
+ MultimapBuilder.hashKeys().arrayListValues().build();
+ codeOwnerSet
+ .codeOwners()
+ .forEach(
+ codeOwnerReference ->
+ codeOwnersByAnnotations.put(
+ codeOwnerSet.annotations().get(codeOwnerReference).stream()
+ .map(CodeOwnerAnnotation::key)
+ .collect(toImmutableSortedSet(naturalOrder())),
+ codeOwnerReference));
+
+ codeOwnersByAnnotations
+ .asMap()
+ .forEach(
+ (annotations, codeOwners) ->
+ b.append(
+ String.format(
+ PER_FILE_LINE_FORMAT,
+ formattedPathExpressions,
+ formatCodeOwnerReferencesAsList(codeOwners)
+ + formatAnnotations(annotations))));
}
return b.toString();
}
private static String formatCodeOwnerReferencesAsList(
- ImmutableSet<CodeOwnerReference> codeOwnerReferences) {
+ Collection<CodeOwnerReference> codeOwnerReferences) {
return formatValuesAsList(codeOwnerReferences.stream().map(CodeOwnerReference::email));
}
+ private static String formatAnnotations(SortedSet<String> annotations) {
+ if (annotations.isEmpty()) {
+ return "";
+ }
+
+ return annotations.stream()
+ .map(annotation -> "#{" + annotation + "}")
+ .collect(joining(" ", " ", ""));
+ }
+
private static String formatValuesAsList(ImmutableSet<String> values) {
return formatValuesAsList(values.stream());
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/findowners/ParsedEmailLine.java b/java/com/google/gerrit/plugins/codeowners/backend/findowners/ParsedEmailLine.java
new file mode 100644
index 0000000..19d5004
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/ParsedEmailLine.java
@@ -0,0 +1,54 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend.findowners;
+
+import static java.util.Objects.requireNonNull;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotation;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
+
+/**
+ * A parsed email line of an {@code OWNERS} file, consisting out of an email and optionally
+ * annotations for that email.
+ */
+@AutoValue
+abstract class ParsedEmailLine {
+ abstract CodeOwnerReference codeOwnerReference();
+
+ abstract ImmutableSet<CodeOwnerAnnotation> annotations();
+
+ static ParsedEmailLine.Builder builder(String email) {
+ requireNonNull(email, "email");
+ return new AutoValue_ParsedEmailLine.Builder()
+ .codeOwnerReference(CodeOwnerReference.create(email));
+ }
+
+ @AutoValue.Builder
+ abstract static class Builder {
+ abstract Builder codeOwnerReference(CodeOwnerReference codeOwnerReference);
+
+ abstract ImmutableSet.Builder<CodeOwnerAnnotation> annotationsBuilder();
+
+ Builder addAnnotation(String annotation) {
+ requireNonNull(annotation, "annotation");
+ annotationsBuilder().add(CodeOwnerAnnotation.create(annotation));
+ return this;
+ }
+
+ abstract ParsedEmailLine build();
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 85db8e3..56aa627 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -16,11 +16,16 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_EXPLICITLY_MENTIONED_SCORING_VALUE;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NOT_EXPLICITLY_MENTIONED_SCORING_VALUE;
import com.google.common.annotations.VisibleForTesting;
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.LinkedListMultimap;
+import com.google.common.collect.ListMultimap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Project;
@@ -34,6 +39,7 @@
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotation;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolverResult;
@@ -200,10 +206,13 @@
int maxDistance = globalOwnersDistance;
CodeOwnerScoring.Builder distanceScoring = CodeOwnerScore.DISTANCE.createScoring(maxDistance);
+ CodeOwnerScoring.Builder isExplicitlyMentionedScoring =
+ CodeOwnerScore.IS_EXPLICITLY_MENTIONED.createScoring();
Set<CodeOwner> codeOwners = new HashSet<>();
+ ListMultimap<CodeOwner, CodeOwnerAnnotation> annotations = LinkedListMultimap.create();
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
- List<String> debugLogs = new ArrayList<>();
+ ImmutableList.Builder<String> debugLogsBuilder = ImmutableList.builder();
codeOwnerConfigHierarchy.visit(
rsrc.getBranch(),
rsrc.getRevision(),
@@ -212,11 +221,9 @@
CodeOwnerResolverResult pathCodeOwners =
codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, rsrc.getPath());
- if (debug) {
- debugLogs.addAll(pathCodeOwners.messages());
- }
-
- codeOwners.addAll(filterCodeOwners(rsrc, pathCodeOwners.codeOwners()));
+ debugLogsBuilder.addAll(pathCodeOwners.messages());
+ codeOwners.addAll(pathCodeOwners.codeOwners());
+ annotations.putAll(pathCodeOwners.annotations());
int distance =
codeOwnerConfig.key().branchNameKey().branch().equals(RefNames.REFS_CONFIG)
@@ -225,14 +232,21 @@
pathCodeOwners
.codeOwners()
.forEach(
- localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
+ localCodeOwner -> {
+ distanceScoring.putValueForCodeOwner(localCodeOwner, distance);
+ isExplicitlyMentionedScoring.putValueForCodeOwner(
+ localCodeOwner, IS_EXPLICITLY_MENTIONED_SCORING_VALUE);
+ });
if (pathCodeOwners.ownedByAllUsers()) {
ownedByAllUsers.set(true);
- ImmutableSet<CodeOwner> addedCodeOwners =
- fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ ImmutableSet<CodeOwner> addedCodeOwners = fillUpWithRandomUsers(codeOwners, limit);
addedCodeOwners.forEach(
- localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
+ localCodeOwner -> {
+ distanceScoring.putValueForCodeOwner(localCodeOwner, distance);
+ isExplicitlyMentionedScoring.putValueForCodeOwner(
+ localCodeOwner, NOT_EXPLICITLY_MENTIONED_SCORING_VALUE);
+ });
if (codeOwners.size() < limit) {
logger.atFine().log(
@@ -251,33 +265,48 @@
return true;
});
- if (codeOwners.size() < limit || !ownedByAllUsers.get()) {
+ if (!ownedByAllUsers.get()) {
CodeOwnerResolverResult globalCodeOwners = getGlobalCodeOwners(rsrc.getBranch().project());
- if (debug) {
- debugLogs.add("resolve global code owners");
- debugLogs.addAll(globalCodeOwners.messages());
- }
+ debugLogsBuilder.add("resolve global code owners");
+ debugLogsBuilder.addAll(globalCodeOwners.messages());
globalCodeOwners
.codeOwners()
.forEach(
- codeOwner -> distanceScoring.putValueForCodeOwner(codeOwner, globalOwnersDistance));
- codeOwners.addAll(filterCodeOwners(rsrc, globalCodeOwners.codeOwners()));
+ codeOwner -> {
+ distanceScoring.putValueForCodeOwner(codeOwner, globalOwnersDistance);
+ isExplicitlyMentionedScoring.putValueForCodeOwner(
+ codeOwner, IS_EXPLICITLY_MENTIONED_SCORING_VALUE);
+ });
+ codeOwners.addAll(globalCodeOwners.codeOwners());
if (globalCodeOwners.ownedByAllUsers()) {
ownedByAllUsers.set(true);
- ImmutableSet<CodeOwner> addedCodeOwners = fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ ImmutableSet<CodeOwner> addedCodeOwners = fillUpWithRandomUsers(codeOwners, limit);
addedCodeOwners.forEach(
- codeOwner -> distanceScoring.putValueForCodeOwner(codeOwner, globalOwnersDistance));
+ codeOwner -> {
+ distanceScoring.putValueForCodeOwner(codeOwner, globalOwnersDistance);
+ isExplicitlyMentionedScoring.putValueForCodeOwner(
+ codeOwner, NOT_EXPLICITLY_MENTIONED_SCORING_VALUE);
+ });
}
}
- ImmutableSet<CodeOwner> immutableCodeOwners = ImmutableSet.copyOf(codeOwners);
+ ImmutableSet<CodeOwner> filteredCodeOwners =
+ filterCodeOwners(
+ rsrc,
+ ImmutableMultimap.copyOf(annotations),
+ ImmutableSet.copyOf(codeOwners),
+ debugLogsBuilder);
CodeOwnerScorings codeOwnerScorings =
- createScorings(rsrc, immutableCodeOwners, distanceScoring.build());
+ createScorings(
+ rsrc,
+ filteredCodeOwners,
+ distanceScoring.build(),
+ isExplicitlyMentionedScoring.build());
ImmutableMap<CodeOwner, Double> scoredCodeOwners =
- codeOwnerScorings.getScorings(immutableCodeOwners);
+ codeOwnerScorings.getScorings(filteredCodeOwners);
ImmutableList<CodeOwner> sortedAndLimitedCodeOwners = sortAndLimit(rsrc, scoredCodeOwners);
@@ -296,14 +325,18 @@
codeOwnersInfo.codeOwners =
codeOwnerJsonFactory.create(getFillOptions()).format(sortedAndLimitedCodeOwners);
codeOwnersInfo.ownedByAllUsers = ownedByAllUsers.get() ? true : null;
+
+ ImmutableList<String> debugLogs = debugLogsBuilder.build();
codeOwnersInfo.debugLogs = debug ? debugLogs : null;
+ logger.atFine().log("debug logs: %s", debugLogs);
+
return Response.ok(codeOwnersInfo);
}
private CodeOwnerScorings createScorings(
- R rsrc, ImmutableSet<CodeOwner> codeOwners, CodeOwnerScoring distanceScoring) {
+ R rsrc, ImmutableSet<CodeOwner> codeOwners, CodeOwnerScoring... scorings) {
ImmutableSet.Builder<CodeOwnerScoring> codeOwnerScorings = ImmutableSet.builder();
- codeOwnerScorings.add(distanceScoring);
+ codeOwnerScorings.addAll(ImmutableSet.copyOf(scorings));
codeOwnerScorings.addAll(getCodeOwnerScorings(rsrc, codeOwners));
return CodeOwnerScorings.create(codeOwnerScorings.build());
}
@@ -343,18 +376,29 @@
* normally doesn't make sense since they will not react to review requests.
* </ul>
*/
- private ImmutableSet<CodeOwner> filterCodeOwners(R rsrc, ImmutableSet<CodeOwner> codeOwners) {
- return filterCodeOwners(rsrc, getVisibleCodeOwners(rsrc, codeOwners)).collect(toImmutableSet());
+ private ImmutableSet<CodeOwner> filterCodeOwners(
+ R rsrc,
+ ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
+ ImmutableSet<CodeOwner> codeOwners,
+ ImmutableList.Builder<String> debugLogs) {
+ return filterCodeOwners(rsrc, annotations, getVisibleCodeOwners(rsrc, codeOwners), debugLogs)
+ .collect(toImmutableSet());
}
/**
* To be overridden by subclasses to filter out additional code owners.
*
* @param rsrc resource on which the request is being performed
+ * @param annotations annotations that were set on the code owners
* @param codeOwners stream of code owners that should be filtered
+ * @param debugLogs builder to collect debug logs that may be returned to the caller
* @return the filtered stream of code owners
*/
- protected Stream<CodeOwner> filterCodeOwners(R rsrc, Stream<CodeOwner> codeOwners) {
+ protected Stream<CodeOwner> filterCodeOwners(
+ R rsrc,
+ ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
+ Stream<CodeOwner> codeOwners,
+ ImmutableList.Builder<String> debugLogs) {
return codeOwners;
}
@@ -484,8 +528,7 @@
*
* @return the added code owners
*/
- private ImmutableSet<CodeOwner> fillUpWithRandomUsers(
- R rsrc, Set<CodeOwner> codeOwners, int limit) {
+ private ImmutableSet<CodeOwner> fillUpWithRandomUsers(Set<CodeOwner> codeOwners, int limit) {
if (!resolveAllUsers || codeOwners.size() >= limit) {
// code ownership for all users should not be resolved or the limit has already been reached
// so that we don't need to add further suggestions
@@ -494,17 +537,12 @@
logger.atFine().log("filling up with random users");
ImmutableSet<CodeOwner> codeOwnersToAdd =
- filterCodeOwners(
- rsrc,
- // ask for 2 times the number of users that we need so that we still have enough
- // suggestions when some users are removed by the filterCodeOwners call or if the
- // returned users were already present in codeOwners
- getRandomVisibleUsers(2 * limit - codeOwners.size())
- .map(CodeOwner::create)
- .collect(toImmutableSet()))
- .stream()
+ // ask for 2 times the number of users that we need so that we still have enough
+ // suggestions when some users are removed on the filter step later or if the returned users
+ // were already present in codeOwners
+ getRandomVisibleUsers(2 * limit - codeOwners.size()).map(CodeOwner::create)
+ .collect(toImmutableSet()).stream()
.filter(codeOwner -> !codeOwners.contains(codeOwner))
- .limit(limit - codeOwners.size())
.collect(toImmutableSet());
codeOwners.addAll(codeOwnersToAdd);
return codeOwnersToAdd;
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
index f4e5c0f..9c4e144 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -14,8 +14,12 @@
package com.google.gerrit.plugins.codeowners.restapi;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.stream.Collectors.toList;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -27,6 +31,7 @@
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerCheckInfo;
import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotations;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
@@ -57,8 +62,10 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.ArrayList;
+import java.util.HashSet;
import java.util.List;
import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
@@ -157,6 +164,7 @@
AtomicBoolean isDefaultCodeOwner = new AtomicBoolean(false);
AtomicBoolean hasRevelantCodeOwnerDefinitions = new AtomicBoolean(false);
AtomicBoolean parentCodeOwnersAreIgnored = new AtomicBoolean(false);
+ Set<String> annotations = new HashSet<>();
codeOwnerConfigHierarchy.visit(
branchResource.getBranchKey(),
ObjectId.fromString(branchResource.getRevision()),
@@ -187,15 +195,23 @@
if (RefNames.isConfigRef(codeOwnerConfig.key().ref())) {
messages.add(
String.format(
- "found email %s as code owner in default code owner config", email));
+ "found email %s as a code owner in the default code owner config", email));
isDefaultCodeOwner.set(true);
} else {
Path codeOwnerConfigFilePath = codeOwners.getFilePath(codeOwnerConfig.key());
messages.add(
String.format(
- "found email %s as code owner in %s", email, codeOwnerConfigFilePath));
+ "found email %s as a code owner in %s", email, codeOwnerConfigFilePath));
codeOwnerConfigFilePaths.add(codeOwnerConfigFilePath);
}
+
+ ImmutableSet<String> localAnnotations =
+ pathCodeOwnersResult.get().getAnnotationsFor(email);
+ if (!localAnnotations.isEmpty()) {
+ messages.add(
+ String.format("email %s is annotated with %s", email, sort(localAnnotations)));
+ annotations.addAll(localAnnotations);
+ }
}
if (pathCodeOwnersResult.get().getPathCodeOwners().stream()
@@ -205,19 +221,31 @@
if (RefNames.isConfigRef(codeOwnerConfig.key().ref())) {
messages.add(
String.format(
- "found email %s as code owner in default code owner config",
- CodeOwnerResolver.ALL_USERS_WILDCARD));
+ "found the all users wildcard ('%s') as a code owner in the default code"
+ + " owner config which makes %s a code owner",
+ CodeOwnerResolver.ALL_USERS_WILDCARD, email));
isDefaultCodeOwner.set(true);
} else {
Path codeOwnerConfigFilePath = codeOwners.getFilePath(codeOwnerConfig.key());
messages.add(
String.format(
- "found email %s as code owner in %s",
- CodeOwnerResolver.ALL_USERS_WILDCARD, codeOwnerConfigFilePath));
+ "found the all users wildcard ('%s') as a code owner in %s which makes %s a"
+ + " code owner",
+ CodeOwnerResolver.ALL_USERS_WILDCARD, codeOwnerConfigFilePath, email));
if (!codeOwnerConfigFilePaths.contains(codeOwnerConfigFilePath)) {
codeOwnerConfigFilePaths.add(codeOwnerConfigFilePath);
}
}
+
+ ImmutableSet<String> localAnnotations =
+ pathCodeOwnersResult.get().getAnnotationsFor(CodeOwnerResolver.ALL_USERS_WILDCARD);
+ if (!localAnnotations.isEmpty()) {
+ messages.add(
+ String.format(
+ "found annotations for the all users wildcard ('%s') which apply to %s: %s",
+ CodeOwnerResolver.ALL_USERS_WILDCARD, email, sort(localAnnotations)));
+ annotations.addAll(localAnnotations);
+ }
}
if (codeOwnerResolverProvider
@@ -282,6 +310,17 @@
}
}
+ ImmutableSet<String> unsupportedAnnotations =
+ annotations.stream()
+ .filter(annotation -> !CodeOwnerAnnotations.isSupported(annotation))
+ .collect(toImmutableSet());
+ if (!unsupportedAnnotations.isEmpty()) {
+ messages.add(
+ String.format(
+ "dropping unsupported annotations for %s: %s", email, sort(unsupportedAnnotations)));
+ annotations.removeAll(unsupportedAnnotations);
+ }
+
boolean isFallbackCodeOwner =
!isCodeOwnershipAssignedToEmail.get()
&& !isCodeOwnershipAssignedToAllUsers.get()
@@ -305,6 +344,7 @@
codeOwnerCheckInfo.isDefaultCodeOwner = isDefaultCodeOwner.get();
codeOwnerCheckInfo.isGlobalCodeOwner = isGlobalCodeOwner;
codeOwnerCheckInfo.isOwnedByAllUsers = isCodeOwnershipAssignedToAllUsers.get();
+ codeOwnerCheckInfo.annotations = sort(annotations);
codeOwnerCheckInfo.debugLogs = messages;
return Response.ok(codeOwnerCheckInfo);
}
@@ -408,4 +448,8 @@
}
return OptionalResultWithMessages.createEmpty(messages);
}
+
+ private ImmutableList<String> sort(Set<String> set) {
+ return set.stream().sorted().collect(toImmutableList());
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index e2aac82..cdab23f 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -17,14 +17,17 @@
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_REVIEWER_SCORING_VALUE;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NO_REVIEWER_SCORING_VALUE;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.common.AccountVisibility;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotation;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotations;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
@@ -53,7 +56,6 @@
*/
public class GetCodeOwnersForPathInChange
extends AbstractGetCodeOwnersForPath<CodeOwnersInChangeCollection.PathResource> {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final ServiceUserClassifier serviceUserClassifier;
@@ -123,31 +125,57 @@
@Override
protected Stream<CodeOwner> filterCodeOwners(
- CodeOwnersInChangeCollection.PathResource rsrc, Stream<CodeOwner> codeOwners) {
- return codeOwners.filter(filterOutChangeOwner(rsrc)).filter(filterOutServiceUsers());
+ CodeOwnersInChangeCollection.PathResource rsrc,
+ ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
+ Stream<CodeOwner> codeOwners,
+ ImmutableList.Builder<String> debugLogs) {
+ return codeOwners
+ .filter(filterOutChangeOwner(rsrc, debugLogs))
+ .filter(filterOutCodeOwnersThatAreAnnotatedWithNeverSuggest(annotations, debugLogs))
+ .filter(filterOutServiceUsers(debugLogs));
}
private Predicate<CodeOwner> filterOutChangeOwner(
- CodeOwnersInChangeCollection.PathResource rsrc) {
+ CodeOwnersInChangeCollection.PathResource rsrc, ImmutableList.Builder<String> debugLogs) {
return codeOwner -> {
if (!codeOwner.accountId().equals(rsrc.getRevisionResource().getChange().getOwner())) {
// Returning true from the Predicate here means that the code owner should be kept.
return true;
}
- logger.atFine().log(
- "Filtering out %s because this code owner is the change owner", codeOwner);
+ debugLogs.add(
+ String.format("filtering out %s because this code owner is the change owner", codeOwner));
// Returning false from the Predicate here means that the code owner should be filtered out.
return false;
};
}
- private Predicate<CodeOwner> filterOutServiceUsers() {
+ private Predicate<CodeOwner> filterOutCodeOwnersThatAreAnnotatedWithNeverSuggest(
+ ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
+ ImmutableList.Builder<String> debugLogs) {
+ return codeOwner -> {
+ boolean neverSuggest =
+ annotations.containsEntry(codeOwner, CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION);
+ if (!neverSuggest) {
+ // Returning true from the Predicate here means that the code owner should be kept.
+ return true;
+ }
+ debugLogs.add(
+ String.format(
+ "filtering out %s because this code owner is annotated with %s",
+ codeOwner, CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION.key()));
+ // Returning false from the Predicate here means that the code owner should be filtered out.
+ return false;
+ };
+ }
+
+ private Predicate<CodeOwner> filterOutServiceUsers(ImmutableList.Builder<String> debugLogs) {
return codeOwner -> {
if (!isServiceUser(codeOwner)) {
// Returning true from the Predicate here means that the code owner should be kept.
return true;
}
- logger.atFine().log("Filtering out %s because this code owner is a service user", codeOwner);
+ debugLogs.add(
+ String.format("filtering out %s because this code owner is a service user", codeOwner));
// Returning false from the Predicate here means that the code owner should be filtered out.
return false;
};
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
index 31fa4e0..3128b7f 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
@@ -133,6 +133,10 @@
check("isOwnedByAllUsers").that(codeOwnerCheckInfo().isOwnedByAllUsers).isFalse();
}
+ public IterableSubject hasAnnotationsThat() {
+ return check("annotations").that(codeOwnerCheckInfo().annotations);
+ }
+
public void hasDebugLogsThatContainAllOf(String... expectedMessages) {
for (String expectedMessage : expectedMessages) {
check("debugLogs").that(codeOwnerCheckInfo().debugLogs).contains(expectedMessage);
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerSetSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerSetSubject.java
index 974289a..8c7d014 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerSetSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerSetSubject.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.codeowners.testing;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertAbout;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerConfigReferenceSubject.codeOwnerConfigReferences;
import static com.google.gerrit.truth.ListSubject.elements;
@@ -22,7 +24,9 @@
import com.google.common.truth.Correspondence;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.IterableSubject;
+import com.google.common.truth.MapSubject;
import com.google.common.truth.Subject;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotation;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
@@ -85,6 +89,19 @@
return hasCodeOwnersThat().comparingElementsUsing(hasEmail());
}
+ public MapSubject hasAnnotationsThat() {
+ return check("codeOwners()")
+ .that(
+ codeOwnerSet().annotations().asMap().entrySet().stream()
+ .collect(
+ toImmutableMap(
+ e -> e.getKey().email(),
+ e ->
+ e.getValue().stream()
+ .map(CodeOwnerAnnotation::key)
+ .collect(toImmutableSet()))));
+ }
+
/**
* Returns an {@link ListSubject} for the imports in the code owner set.
*
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
index b7f856a..85f52b9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -77,7 +77,7 @@
@Inject private GroupOperations groupOperations;
@Inject private ProjectOperations projectOperations;
- private TestPathExpressions testPathExpressions;
+ protected TestPathExpressions testPathExpressions;
@Before
public void setup() throws Exception {
@@ -1472,7 +1472,7 @@
}
@Test
- public void debugRequireCallerToBeAdminOrHaveTheCheckCodeOwnerCapability() throws Exception {
+ public void debugRequiresCallerToBeAdminOrHaveTheCheckCodeOwnerCapability() throws Exception {
requestScopeOperations.setApiUser(user.id());
AuthException authException =
assertThrows(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
index 0f12736..4baef37 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -23,6 +23,7 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
@@ -44,6 +45,8 @@
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestCodeOwnerConfigCreation;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestPathExpressions;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerCheckInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotation;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotations;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference;
@@ -144,10 +147,11 @@
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
assertThat(checkCodeOwnerInfo).isNotOwnedByAllUsers();
+ assertThat(checkCodeOwnerInfo).hasAnnotationsThat().isEmpty();
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
String.format("resolved email %s to account %s", codeOwner.email(), codeOwner.id()));
}
@@ -178,13 +182,13 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath("/foo/bar/")),
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format("resolved email %s to account %s", codeOwner.email(), codeOwner.id()));
}
@@ -222,10 +226,10 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath("/foo/bar/")),
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
"parent code owners are ignored",
String.format("resolved email %s to account %s", codeOwner.email(), codeOwner.id()));
@@ -257,7 +261,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
secondaryEmail, getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format("resolved email %s to account %s", secondaryEmail, codeOwner.id()));
}
@@ -282,8 +286,11 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
- CodeOwnerResolver.ALL_USERS_WILDCARD, getCodeOwnerConfigFilePath(ROOT_PATH)));
+ "found the all users wildcard ('%s') as a code owner in %s which makes %s a code"
+ + " owner",
+ CodeOwnerResolver.ALL_USERS_WILDCARD,
+ getCodeOwnerConfigFilePath(ROOT_PATH),
+ codeOwner.email()));
}
@Test
@@ -306,11 +313,14 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
- "found email %s as code owner in %s",
- CodeOwnerResolver.ALL_USERS_WILDCARD, getCodeOwnerConfigFilePath(ROOT_PATH)));
+ "found the all users wildcard ('%s') as a code owner in %s which makes %s a code"
+ + " owner",
+ CodeOwnerResolver.ALL_USERS_WILDCARD,
+ getCodeOwnerConfigFilePath(ROOT_PATH),
+ codeOwner.email()));
}
@Test
@@ -345,7 +355,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
nonExistingEmail, getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
"cannot resolve code owner email %s: no account with this email exists",
@@ -374,7 +384,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
ambiguousEmail, getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
"cannot resolve code owner email %s: email is ambiguous", ambiguousEmail));
@@ -406,7 +416,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
orphanedEmail, getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
"cannot resolve account %s for email %s: account does not exists",
@@ -437,7 +447,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
inactiveUser.email(), getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
"ignoring inactive account %s for email %s",
@@ -469,7 +479,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
emailWithAllowedEmailDomain, getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
"domain %s of email %s is allowed",
@@ -503,7 +513,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
emailWithNonAllowedEmailDomain, getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
"domain %s of email %s is not allowed",
@@ -544,8 +554,10 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
- CodeOwnerResolver.ALL_USERS_WILDCARD, getCodeOwnerConfigFilePath(ROOT_PATH)));
+ "found the all users wildcard ('%s') as a code owner in %s which makes %s a code owner",
+ CodeOwnerResolver.ALL_USERS_WILDCARD,
+ getCodeOwnerConfigFilePath(ROOT_PATH),
+ CodeOwnerResolver.ALL_USERS_WILDCARD));
}
@Test
@@ -569,7 +581,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in default code owner config",
+ "found email %s as a code owner in the default code owner config",
defaultCodeOwner.email()),
String.format(
"resolved email %s to account %s",
@@ -597,8 +609,9 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in default code owner config",
- CodeOwnerResolver.ALL_USERS_WILDCARD),
+ "found the all users wildcard ('%s') as a code owner in the default code owner"
+ + " config which makes %s a code owner",
+ CodeOwnerResolver.ALL_USERS_WILDCARD, defaultCodeOwner.email()),
String.format(
"resolved email %s to account %s",
defaultCodeOwner.email(), defaultCodeOwner.id()));
@@ -677,7 +690,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
String.format("account %s is visible to user %s", codeOwner.id(), user.username()),
String.format("resolved email %s to account %s", codeOwner.email(), codeOwner.id()));
@@ -716,7 +729,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
"cannot resolve code owner email %s: account %s is not visible to user %s",
@@ -749,7 +762,7 @@
assertThat(checkCodeOwnerInfo)
.hasDebugLogsThatContainAllOf(
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
secondaryEmail, getCodeOwnerConfigFilePath(ROOT_PATH)),
String.format(
"cannot resolve code owner email %s: account %s is referenced by secondary email"
@@ -935,7 +948,7 @@
"per-file code owner set with path expressions [%s] matches",
testPathExpressions.matchFileType("md")),
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
mdOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
String.format("resolved email %s to account %s", mdOwner.email(), mdOwner.id()));
assertThat(checkCodeOwnerInfo)
@@ -1006,7 +1019,7 @@
+ " parent code owners, hence ignoring the folder code owners",
testPathExpressions.matchFileType("md")),
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
fileCodeOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
String.format(
"resolved email %s to account %s", fileCodeOwner.email(), fileCodeOwner.id()));
@@ -1061,7 +1074,7 @@
getCodeOwnerConfigFileName(),
getCodeOwnerConfigFileName()),
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
codeOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
String.format("resolved email %s to account %s", codeOwner.email(), codeOwner.id()));
}
@@ -1125,7 +1138,7 @@
getCodeOwnerConfigFileName(),
testPathExpressions.matchFileType("md")),
String.format(
- "found email %s as code owner in %s",
+ "found email %s as a code owner in %s",
mdCodeOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
String.format(
"resolved email %s to account %s", mdCodeOwner.email(), mdCodeOwner.id()));
@@ -1454,6 +1467,92 @@
assertThat(checkCodeOwnerInfo).cannotApproveChange();
}
+ @Test
+ public void checkCodeOwnerWithAnnotations() throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(codeOwner.email())
+ .addAnnotation(codeOwner.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .build())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD)
+ .addAnnotation(
+ CodeOwnerResolver.ALL_USERS_WILDCARD, CodeOwnerAnnotation.create("ANNOTATION"))
+ .build())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(codeOwner.email())
+ .addAnnotation(codeOwner.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addAnnotation(codeOwner.email(), CodeOwnerAnnotation.create("OTHER_ANNOTATION"))
+ .build())
+ .create();
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, codeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo)
+ .hasAnnotationsThat()
+ .containsExactly(CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION.key())
+ .inOrder();
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "found email %s as a code owner in %s",
+ codeOwner.email(), getCodeOwnerConfigFilePath("/foo/bar/")),
+ String.format(
+ "email %s is annotated with %s",
+ codeOwner.email(),
+ ImmutableSet.of(
+ CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION.key(), "OTHER_ANNOTATION")),
+ String.format(
+ "found the all users wildcard ('%s') as a code owner in %s which makes %s a code"
+ + " owner",
+ CodeOwnerResolver.ALL_USERS_WILDCARD,
+ getCodeOwnerConfigFilePath("/foo/"),
+ codeOwner.email()),
+ String.format(
+ "found annotations for the all users wildcard ('%s') which apply to %s: %s",
+ CodeOwnerResolver.ALL_USERS_WILDCARD,
+ codeOwner.email(),
+ ImmutableSet.of("ANNOTATION")),
+ String.format(
+ "found email %s as a code owner in %s",
+ codeOwner.email(), getCodeOwnerConfigFilePath("/")),
+ String.format(
+ "email %s is annotated with %s",
+ codeOwner.email(),
+ ImmutableSet.of(CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION.key())),
+ String.format(
+ "dropping unsupported annotations for %s: %s",
+ codeOwner.email(), ImmutableSet.of("ANNOTATION", "OTHER_ANNOTATION")));
+ }
+
private CodeOwnerCheckInfo checkCodeOwner(String path, String email) throws RestApiException {
return checkCodeOwner(path, email, /* user= */ null);
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
index c260776..82ca271 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
@@ -32,7 +32,9 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.api.CodeOwners;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotations;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSetModification;
import com.google.inject.Inject;
import java.util.List;
@@ -243,4 +245,63 @@
.comparingElementsUsing(hasAccountId())
.containsExactly(serviceUser.id());
}
+
+ @Test
+ public void codeOwnersWithNeverSuggestAnnotationAreIncluded() throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
+ .build())
+ .create();
+
+ // Expectation: admin is included because GetCodeOwnersForPathInBranch ignores the NEVER_SUGGEST
+ // suggestion.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id(), user2.id());
+ }
+
+ @Test
+ public void perFileCodeOwnersWithNeverSuggestAnnotationAreIncluded() throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
+ .build())
+ .create();
+
+ // Expectation: admin is included because GetCodeOwnersForPathInBranch ignores the NEVER_SUGGEST
+ // suggestion.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id(), user2.id());
+ }
}
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 1feefc6..d5eeb0c 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -36,7 +36,10 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.api.CodeOwners;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotations;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
import java.util.List;
@@ -327,6 +330,211 @@
}
@Test
+ public void codeOwnersWithNeverSuggestAnnotationAreFilteredOut() throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
+ .build())
+ .create();
+
+ // Expectation: admin is filtered out because it is annotated with NEVER_SUGGEST.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id(), user2.id());
+ }
+
+ @Test
+ public void codeOwnersWithNeverSuggestAnnotationAreFilteredOut_annotationSetForAllUsersWildcard()
+ throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD)
+ .addAnnotation(
+ CodeOwnerResolver.ALL_USERS_WILDCARD,
+ CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
+ .build())
+ .create();
+
+ // Expectation: none of the code owners is suggested since NEVER_SUGGEST was set for the all
+ // users wildcard.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty();
+ }
+
+ @Test
+ public void perFileCodeOwnersWithNeverSuggestAnnotationAreFilteredOut() throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
+ .build())
+ .create();
+
+ // Expectation: admin is filtered out because it is annotated with NEVER_SUGGEST.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id(), user2.id());
+ }
+
+ @Test
+ public void
+ perFileCodeOwnersWithNeverSuggestAnnotationAreFilteredOut_annotationSetForAllUsersWildcard()
+ throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD)
+ .addAnnotation(
+ CodeOwnerResolver.ALL_USERS_WILDCARD,
+ CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
+ .build())
+ .create();
+
+ // Expectation: none of the code owners is suggested since NEVER_SUGGEST was set for the all
+ // users wildcard.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty();
+ }
+
+ @Test
+ public void neverSuggestTakesEffectEvenIfCodeOwnerIsAlsoSpecifiedWithoutThisAnnotation()
+ throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ // Code owner config with admin as code owner, but without annotation.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ // Code owner config that specifies admin multiple times as code owner, but only once with the
+ // NEVER_SUGGEST annotation.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
+ .build())
+ // Another code owner set with admin as folder code owner, but without annotation.
+ .addCodeOwnerSet(CodeOwnerSet.builder().addCodeOwnerEmail(admin.email()).build())
+ // A per-file code owner with admin as file code owner, but without annotation.
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .addCodeOwnerEmail(admin.email())
+ .build())
+ .create();
+
+ // Another code owner config with admin as code owner, but without annotation.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ // Expectation: admin is filtered out because at once place it is annotated with NEVER_SUGGEST.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id(), user2.id());
+ }
+
+ @Test
+ public void neverSuggestOnNonMatchingPerFileRuleDoesntHaveAnyEffect() throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
+ .build())
+ // Non-matching per-file code owner with NEVER_SUGGEST annotation.
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("txt"))
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .build())
+ .create();
+
+ // Expectation: admin is suggested since the NEVER_SUGGEST annotation is set on the per-file
+ // rule which doesn't match.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id(), user2.id());
+ }
+
+ @Test
public void codeOwnersThatAreReviewersAreReturnedFirst() throws Exception {
TestAccount user2 = accountCreator.user2();
@@ -429,4 +637,52 @@
.inOrder();
assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue();
}
+
+ @Test
+ public void filteredOutCodeOwnersAreMentionedInDebugLogs() throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ // Create a service user.
+ TestAccount serviceUser =
+ accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
+ groupOperations
+ .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
+ .forUpdate()
+ .addMember(serviceUser.id())
+ .update();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(changeOwner.email())
+ .addCodeOwnerEmail(serviceUser.email())
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(user.email())
+ .build())
+ .create();
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().withDebug(/* debug= */ true), path);
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id());
+ assertThat(codeOwnersInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "filtering out %s because this code owner is the change owner",
+ CodeOwner.create(changeOwner.id())),
+ String.format(
+ "filtering out %s because this code owner is a service user",
+ CodeOwner.create(serviceUser.id())),
+ String.format(
+ "filtering out %s because this code owner is annotated with %s",
+ CodeOwner.create(admin.id()), CodeOwnerAnnotations.NEVER_SUGGEST_ANNOTATION.key()));
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResultTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResultTest.java
index d64f5bf..18ea017 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResultTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResultTest.java
@@ -15,6 +15,7 @@
package com.google.gerrit.plugins.codeowners.backend;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import org.junit.Test;
@@ -25,6 +26,7 @@
CodeOwnerResolverResult codeOwnerResolverResult =
CodeOwnerResolverResult.create(
ImmutableSet.of(CodeOwner.create(admin.id())),
+ /* annotations= */ ImmutableMultimap.of(),
/* ownedByAllUsers= */ false,
/* hasUnresolvedCodeOwners= */ false,
/* hasUnresolvedImports= */ false,
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 629f42c..6e91160 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -426,6 +426,65 @@
}
@Test
+ public void resolvePathCodeOwnersWithAnnotations() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ CodeOwnerConfig codeOwnerConfig =
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotation.create("FOO"))
+ .addAnnotation(admin.email(), CodeOwnerAnnotation.create("BAR"))
+ .addCodeOwnerEmail(user.email())
+ .addAnnotation(user.email(), CodeOwnerAnnotation.create("BAZ"))
+ .addCodeOwnerEmail(user2.email())
+ .build())
+ .build();
+
+ CodeOwnerResolverResult result =
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id(), user.id(), user2.id());
+ assertThat(result.annotations().keySet())
+ .containsExactly(CodeOwner.create(admin.id()), CodeOwner.create(user.id()));
+ assertThat(result.annotations().get(CodeOwner.create(admin.id())))
+ .containsExactly(CodeOwnerAnnotation.create("FOO"), CodeOwnerAnnotation.create("BAR"));
+ assertThat(result.annotations().get(CodeOwner.create(user.id())))
+ .containsExactly(CodeOwnerAnnotation.create("BAZ"));
+ }
+
+ @Test
+ public void resolvePathCodeOwnersWithAnnotations_annotationOnAllUsersWildcard()
+ throws Exception {
+ CodeOwnerConfig codeOwnerConfig =
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), CodeOwnerAnnotation.create("FOO"))
+ .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD)
+ .addAnnotation(
+ CodeOwnerResolver.ALL_USERS_WILDCARD, CodeOwnerAnnotation.create("BAR"))
+ .addCodeOwnerEmail(user.email())
+ .build())
+ .build();
+
+ CodeOwnerResolverResult result =
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id(), user.id());
+ assertThat(result.annotations().keySet())
+ .containsExactly(CodeOwner.create(admin.id()), CodeOwner.create(user.id()));
+ assertThat(result.annotations().get(CodeOwner.create(admin.id())))
+ .containsExactly(CodeOwnerAnnotation.create("FOO"), CodeOwnerAnnotation.create("BAR"));
+ assertThat(result.annotations().get(CodeOwner.create(user.id())))
+ .containsExactly(CodeOwnerAnnotation.create("BAR"));
+ }
+
+ @Test
public void cannotResolvePathCodeOwnersOfNullCodeOwnerConfig() throws Exception {
NullPointerException npe =
assertThrows(
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 53c986d..cc9cefc 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
@@ -22,6 +22,7 @@
import static java.util.stream.Collectors.joining;
import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.plugins.codeowners.backend.AbstractCodeOwnerConfigParserTest;
@@ -181,6 +182,47 @@
}
@Test
+ public void codeOwnerConfigWithAnnotations() throws Exception {
+ assertParseAndFormat(
+ getCodeOwnerConfig(
+ EMAIL_1,
+ EMAIL_2 + " #{FOO_BAR}#{BAR_BAZ} #NO_ANNOTATION, #{FOO} #{bar} #{bAz} other comment",
+ EMAIL_3),
+ codeOwnerConfig -> {
+ CodeOwnerSetSubject codeOwnerSetSubject =
+ assertThat(codeOwnerConfig).hasCodeOwnerSetsThat().onlyElement();
+ codeOwnerSetSubject.hasCodeOwnersEmailsThat().containsExactly(EMAIL_1, EMAIL_2, EMAIL_3);
+ codeOwnerSetSubject
+ .hasAnnotationsThat()
+ .containsExactly(EMAIL_2, ImmutableSet.of("FOO_BAR", "BAR_BAZ", "FOO", "bar", "bAz"));
+ },
+ // annotations are sorted alphabetically, the normal comment is dropped
+ EMAIL_1
+ + "\n"
+ + EMAIL_2
+ + " #{BAR_BAZ} #{FOO} #{FOO_BAR} #{bAz} #{bar}\n"
+ + EMAIL_3
+ + "\n");
+ }
+
+ @Test
+ public void codeOwnerConfigWithAnnotationsOnAllUsersWildcard() throws Exception {
+ assertParseAndFormat(
+ getCodeOwnerConfig(
+ "* #{FOO_BAR}#{BAR_BAZ} #NO_ANNOTATION, #{FOO} #{bar} #{bAz} other comment"),
+ codeOwnerConfig -> {
+ CodeOwnerSetSubject codeOwnerSetSubject =
+ assertThat(codeOwnerConfig).hasCodeOwnerSetsThat().onlyElement();
+ codeOwnerSetSubject.hasCodeOwnersEmailsThat().containsExactly("*");
+ codeOwnerSetSubject
+ .hasAnnotationsThat()
+ .containsExactly("*", ImmutableSet.of("FOO_BAR", "BAR_BAZ", "FOO", "bar", "bAz"));
+ },
+ // annotations are sorted alphabetically, the normal comment is dropped
+ "* #{BAR_BAZ} #{FOO} #{FOO_BAR} #{bAz} #{bar}\n");
+ }
+
+ @Test
public void codeOwnerConfigWithNonSortedEmails() throws Exception {
assertParseAndFormat(
String.join("\n", EMAIL_3, EMAIL_2, EMAIL_1) + "\n",
@@ -428,6 +470,41 @@
}
@Test
+ public void perFileCodeOwnerConfigWithAnnotations() throws Exception {
+ assertParseAndFormat(
+ "per-file foo="
+ + EMAIL_1
+ + ","
+ + EMAIL_2
+ + ","
+ + EMAIL_3
+ + " #{FOO_BAR}#{BAR_BAZ} #NO_ANNOTATION, #{FOO} #{bar} #{bAz} other comment",
+ codeOwnerConfig -> {
+ CodeOwnerSetSubject codeOwnerSetSubject =
+ assertThat(codeOwnerConfig).hasCodeOwnerSetsThat().onlyElement();
+ codeOwnerSetSubject.hasPathExpressionsThat().containsExactly("foo");
+ codeOwnerSetSubject.hasCodeOwnersEmailsThat().containsExactly(EMAIL_1, EMAIL_2, EMAIL_3);
+ codeOwnerSetSubject
+ .hasAnnotationsThat()
+ .containsExactly(
+ EMAIL_1,
+ ImmutableSet.of("FOO_BAR", "BAR_BAZ", "FOO", "bar", "bAz"),
+ EMAIL_2,
+ ImmutableSet.of("FOO_BAR", "BAR_BAZ", "FOO", "bar", "bAz"),
+ EMAIL_3,
+ ImmutableSet.of("FOO_BAR", "BAR_BAZ", "FOO", "bar", "bAz"));
+ },
+ // annotations are sorted alphabetically, the normal comment is dropped, a newline is added
+ "per-file foo="
+ + EMAIL_1
+ + ","
+ + EMAIL_2
+ + ","
+ + EMAIL_3
+ + " #{BAR_BAZ} #{FOO} #{FOO_BAR} #{bAz} #{bar}\n");
+ }
+
+ @Test
public void perFileCodeOwnerConfigImportFromSameProjectAndBranch() throws Exception {
Path path = Paths.get("/foo/bar/OWNERS");
CodeOwnerConfigReference codeOwnerConfigReference =
diff --git a/resources/Documentation/backend-find-owners.md b/resources/Documentation/backend-find-owners.md
index a1af71d..122b598 100644
--- a/resources/Documentation/backend-find-owners.md
+++ b/resources/Documentation/backend-find-owners.md
@@ -291,6 +291,41 @@
per-file docs.config,*.md=richard.roe@example.com
```
+### <a id="anotations">Annotations
+
+Lines representing [access grants](#accessGrants) can be annotated. Annotations
+have the format `#{ANNOTATION_NAME}` and can appear at the end of the line.
+E.g.:
+
+```
+ john.doe@example.com #{NEVER_SUGGEST}
+ per-file docs.config,*.md=richard.roe@example.com #{NEVER_SUGGEST}
+```
+\
+Annotations can be mixed with [comments](#comments) that can appear before and
+after annotations, E.g.:
+
+```
+ jane.roe@example.com # foo bar #{NEVER_SUGGEST} baz
+```
+\
+The following annotations are supported:
+
+#### <a id="neverSuggest">
+* `NEVER_SUGGEST`:
+ Code owners with this annotation are omitted when [suggesting code
+ owners](rest-api.html#list-code-owners-for-path-in-change). If code ownership
+ is assigned to the same code owner through multiple relevant access grants in
+ the same code owner config file or in other relevant code owner config files
+ the code owner gets omitted from the suggestion if it has the `NEVER_SUGGEST`
+ set on any of the access grants.
+
+Unknown annotations are silently ignored.
+
+**NOTE:** If an access grant line that assigns code ownership to multiple users
+has an annotation, this annotation applies to all these users. E.g. if an
+annotation is set for the all users wildcard (aka `*`) it applies to all users.
+
### <a id="comments">Comments
The '#' character indicates the beginning of a comment. Arbitrary text may be
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 461232e..90d6055 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -483,6 +483,9 @@
* distance of the code owner config file that defines the code owner to the
path for which code owners are listed (the lower the distance the better the
code owner)
+* whether the user is explicitly mentioned as a code owner in the code owner
+ config file vs. the user being a code owner only because the code ownership
+ has been assigned to all users (aka `*`)
* whether the code owner is a reviewer of the change (only when listing code
owners for a change)
@@ -673,6 +676,8 @@
* [service users](#serviceUsers) (members of the `Service Users` group)
* the change owner (since the change owner cannot be added as reviewer)
+* code owners that are annotated with
+ [NEVER_SUGGEST](backend-find-owners.html#neverSuggest)
In addition, by default the change number is used as seed if none was specified.
This way the sort order on a change is always the same for files that have the
@@ -842,6 +847,7 @@
| `is_global_code_owner` | Whether the given email is configured as a global
code owner. Note that if the email is configured as global code owner, but the email is not resolvable (see `is_resolvable` field), the user is not a code owner.
| `is_owned_by_all_users` | Whether the the specified path in the branch is owned by all users (aka `*`).
+| `annotation` | Annotations that were set for the user. Contains only supported annotations (unsupported annotations are reported in the `debugs_logs`). Sorted alphabetically.
| `debug_logs` | List of debug logs that may help to understand why the user is or isn't a code owner.
---
