CheckCodeOwner: Inform in debug logs about matching per-file rules
So far the CheckCodeOwner REST endpoint only tells if a user is a code
owner and through which code owner config file the code ownership is
assigned, but not which of the per-file rules in the code owner config
file matched. Include this info to make debugging easier.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I7bbf9a3136ada646a4f658e4599d82cbe261fd3e
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
index 26578fc..e42501d 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
@@ -97,6 +97,11 @@
assumeThatCodeOwnersBackendIsNotProtoBackend();
}
+ protected void skipTestIfIgnoreParentCodeOwnersNotSupportedByCodeOwnersBackend() {
+ // the proto backend doesn't support ignoring parent code owners
+ assumeThatCodeOwnersBackendIsNotProtoBackend();
+ }
+
protected void assumeThatCodeOwnersBackendIsNotProtoBackend() {
assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index 443b5ef..05843d1 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -34,6 +34,7 @@
import com.google.inject.Singleton;
import java.nio.file.Path;
import java.util.ArrayDeque;
+import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
@@ -202,13 +203,22 @@
CodeOwnerConfig.Builder resolvedCodeOwnerConfigBuilder =
CodeOwnerConfig.builder(codeOwnerConfig.key(), codeOwnerConfig.revision());
+ List<String> messages = new ArrayList<>();
+
// Add all data from the importing code owner config.
resolvedCodeOwnerConfigBuilder.setIgnoreParentCodeOwners(
codeOwnerConfig.ignoreParentCodeOwners());
getGlobalCodeOwnerSets(codeOwnerConfig)
.forEach(resolvedCodeOwnerConfigBuilder::addCodeOwnerSet);
getMatchingPerFileCodeOwnerSets(codeOwnerConfig)
- .forEach(resolvedCodeOwnerConfigBuilder::addCodeOwnerSet);
+ .forEach(
+ codeOwnerSet -> {
+ messages.add(
+ String.format(
+ "per-file code owner set with path expressions %s matches",
+ codeOwnerSet.pathExpressions()));
+ resolvedCodeOwnerConfigBuilder.addCodeOwnerSet(codeOwnerSet);
+ });
List<UnresolvedImport> unresolvedImports =
resolveImports(codeOwnerConfig, resolvedCodeOwnerConfigBuilder);
@@ -219,9 +229,19 @@
// ignoreGlobalAndParentCodeOwners flag set to true.
// In this case also set ignoreParentCodeOwners to true, so that we do not need to inspect the
// ignoreGlobalAndParentCodeOwners flags again.
- if (getMatchingPerFileCodeOwnerSets(resolvedCodeOwnerConfig)
- .anyMatch(CodeOwnerSet::ignoreGlobalAndParentCodeOwners)) {
- logger.atFine().log("remove global code owner sets and set ignoreParentCodeOwners to true");
+ Optional<CodeOwnerSet> matchingPerFileCodeOwnerSetThatIgnoresGlobalAndParentCodeOwners =
+ getMatchingPerFileCodeOwnerSets(resolvedCodeOwnerConfig)
+ .filter(CodeOwnerSet::ignoreGlobalAndParentCodeOwners)
+ .findAny();
+ if (matchingPerFileCodeOwnerSetThatIgnoresGlobalAndParentCodeOwners.isPresent()) {
+ logger.atFine().log("remove folder code owner sets and set ignoreParentCodeOwners to true");
+ messages.add(
+ String.format(
+ "found matching per-file code owner set (with path expressions = %s) that ignores"
+ + " parent code owners, hence ignoring the folder code owners",
+ matchingPerFileCodeOwnerSetThatIgnoresGlobalAndParentCodeOwners
+ .get()
+ .pathExpressions()));
resolvedCodeOwnerConfig =
resolvedCodeOwnerConfig
.toBuilder()
@@ -236,7 +256,7 @@
this.pathCodeOwnersResult =
OptionalResultWithMessages.create(
PathCodeOwnersResult.create(path, resolvedCodeOwnerConfig, unresolvedImports),
- ImmutableList.of());
+ messages);
logger.atFine().log("path code owners result = %s", pathCodeOwnersResult);
return this.pathCodeOwnersResult;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
index b060198..f9f8e7b 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
@@ -87,6 +87,12 @@
}
}
+ public void hasDebugLogsThatDoNotContainAnyOf(String... expectedMessages) {
+ for (String expectedMessage : expectedMessages) {
+ check("debugLogs").that(codeOwnerCheckInfo().debugLogs).doesNotContain(expectedMessage);
+ }
+ }
+
public IterableSubject hasDebugLogsThat() {
return check("debugLogs").that(codeOwnerCheckInfo().debugLogs);
}
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 8166970..4964186 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -31,12 +31,14 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestPathExpressions;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerCheckInfo;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
@@ -67,10 +69,12 @@
@Inject private ExternalIdNotes.Factory externalIdNotesFactory;
private BackendConfig backendConfig;
+ private TestPathExpressions testPathExpressions;
@Before
public void setUpCodeOwnersPlugin() throws Exception {
backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class);
+ testPathExpressions = plugin.getSysInjector().getInstance(TestPathExpressions.class);
}
@Test
@@ -641,6 +645,122 @@
projectOperations.project(project).getHead("master").name()));
}
+ @Test
+ public void checkPerFileCodeOwner() throws Exception {
+ TestAccount txtOwner =
+ accountCreator.create(
+ "txtCodeOwner", "txtCodeOwner@example.com", "Txt Code Owner", /* displayName= */ null);
+ TestAccount mdOwner =
+ accountCreator.create(
+ "mdCodeOwner", "mdCodeOwner@example.com", "Md Code Owner", /* displayName= */ null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("txt"))
+ .addCodeOwnerEmail(txtOwner.email())
+ .build())
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .addCodeOwnerEmail(mdOwner.email())
+ .build())
+ .create();
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, mdOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo)
+ .hasCodeOwnerConfigFilePathsThat()
+ .containsExactly(getCodeOwnerConfigFilePath("/foo/"));
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "per-file code owner set with path expressions [%s] matches",
+ testPathExpressions.matchFileType("md")),
+ String.format(
+ "found email %s as code owner in %s",
+ mdOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
+ String.format("resolved to account %s", mdOwner.id()));
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatDoNotContainAnyOf(
+ String.format(
+ "per-file code owner set with path expressions [%s] matches",
+ testPathExpressions.matchFileType("txt")));
+ }
+
+ @Test
+ public void checkPerFileCodeOwnerWhenParentCodeOwnersAreIgnored() throws Exception {
+ skipTestIfIgnoreParentCodeOwnersNotSupportedByCodeOwnersBackend();
+
+ TestAccount fileCodeOwner =
+ accountCreator.create(
+ "fileCodeOwner",
+ "fileCodeOwner@example.com",
+ "File Code Owner",
+ /* displayName= */ null);
+ TestAccount folderCodeOwner =
+ accountCreator.create(
+ "folderCodeOwner",
+ "folderCodeOwner@example.com",
+ "Folder Code Owner",
+ /* displayName= */ null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(folderCodeOwner.email())
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .setIgnoreGlobalAndParentCodeOwners()
+ .addCodeOwnerEmail(fileCodeOwner.email())
+ .build())
+ .create();
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, folderCodeOwner.email());
+ assertThat(checkCodeOwnerInfo).isNotCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo).hasCodeOwnerConfigFilePathsThat().isEmpty();
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "per-file code owner set with path expressions [%s] matches",
+ testPathExpressions.matchFileType("md")),
+ String.format(
+ "found matching per-file code owner set (with path expressions = [%s]) that ignores"
+ + " parent code owners, hence ignoring the folder code owners",
+ testPathExpressions.matchFileType("md")));
+
+ checkCodeOwnerInfo = checkCodeOwner(path, fileCodeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo)
+ .hasCodeOwnerConfigFilePathsThat()
+ .containsExactly(getCodeOwnerConfigFilePath("/foo/"));
+ assertThat(checkCodeOwnerInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "per-file code owner set with path expressions [%s] matches",
+ testPathExpressions.matchFileType("md")),
+ String.format(
+ "found matching per-file code owner set (with path expressions = [%s]) that ignores"
+ + " parent code owners, hence ignoring the folder code owners",
+ testPathExpressions.matchFileType("md")),
+ String.format(
+ "found email %s as code owner in %s",
+ fileCodeOwner.email(), getCodeOwnerConfigFilePath("/foo/")),
+ String.format("resolved to account %s", fileCodeOwner.id()));
+ }
+
private CodeOwnerCheckInfo checkCodeOwner(String path, String email) throws RestApiException {
return checkCodeOwner(path, email, null);
}
