Do not apply fallback code owners if inheritance of parent code owners is disabled
If there is a relevant code owner config file that explicitly ignores
parent code owners the fallback code owners should not be applied even
if there are no code owners defined for the path.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I5a7cefe803bb72b45b01fc28856bd4e2b375bc08
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 436c45e..f868c42 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -501,6 +501,7 @@
}
AtomicBoolean hasRevelantCodeOwnerDefinitions = new AtomicBoolean(false);
+ AtomicBoolean parentCodeOwnersAreIgnored = new AtomicBoolean(false);
codeOwnerConfigHierarchy.visit(
branch,
revision,
@@ -536,13 +537,20 @@
// We need to continue to check if any of the higher-level code owners approved the
// change or is a reviewer.
return true;
+ },
+ codeOwnerConfigKey -> {
+ logger.atFine().log(
+ "code owner config %s ignores parent code owners for %s",
+ codeOwnerConfigKey, absolutePath);
+ parentCodeOwnersAreIgnored.set(true);
});
- // If no code owners have been defined for the file, the fallback code owners apply if they
- // are configured. We can skip checking them if we already found that the file was
- // approved.
+ // If no code owners have been defined for the file and if parent code owners are not ignored,
+ // the fallback code owners apply if they are configured. We can skip checking them if we
+ // already found that the file was approved.
if (codeOwnerStatus.get() != CodeOwnerStatus.APPROVED
- && !hasRevelantCodeOwnerDefinitions.get()) {
+ && !hasRevelantCodeOwnerDefinitions.get()
+ && !parentCodeOwnersAreIgnored.get()) {
codeOwnerStatus.set(
getCodeOwnerStatusForFallbackCodeOwners(
codeOwnerStatus.get(),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index e3bbb85..f443c50 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -28,6 +28,7 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.Optional;
+import java.util.function.Consumer;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -74,10 +75,38 @@
ObjectId revision,
Path absolutePath,
CodeOwnerConfigVisitor codeOwnerConfigVisitor) {
+ visit(
+ branchNameKey,
+ revision,
+ absolutePath,
+ codeOwnerConfigVisitor,
+ /* parentCodeOwnersIgnoredCallback= */ codeOwnerConfigKey -> {});
+ }
+
+ /**
+ * Visits the code owner configs in the given branch that apply for the given path by following
+ * the path hierarchy from the given path up to the root folder.
+ *
+ * @param branchNameKey project and branch from which the code owner configs should be visited
+ * @param revision the branch revision from which the code owner configs should be loaded
+ * @param absolutePath the path for which the code owner configs should be visited; the path must
+ * be absolute; can be the path of a file or folder; the path may or may not exist
+ * @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner
+ * configs
+ * @param parentCodeOwnersIgnoredCallback callback that is invoked for the first visited code
+ * owner config that ignores parent code owners
+ */
+ public void visit(
+ BranchNameKey branchNameKey,
+ ObjectId revision,
+ Path absolutePath,
+ CodeOwnerConfigVisitor codeOwnerConfigVisitor,
+ Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
requireNonNull(branchNameKey, "branch");
requireNonNull(revision, "revision");
requireNonNull(absolutePath, "absolutePath");
requireNonNull(codeOwnerConfigVisitor, "codeOwnerConfigVisitor");
+ requireNonNull(parentCodeOwnersIgnoredCallback, "parentCodeOwnersIgnoredCallback");
checkState(absolutePath.isAbsolute(), "path %s must be absolute", absolutePath);
logger.atFine().log(
@@ -93,14 +122,18 @@
// Read code owner config and invoke the codeOwnerConfigVisitor if the code owner config
// exists.
logger.atFine().log("inspecting code owner config for %s", ownerConfigFolder);
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(
- CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder), revision, absolutePath);
+ pathCodeOwnersFactory.create(codeOwnerConfigKey, revision, absolutePath);
if (pathCodeOwners.isPresent()) {
logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
boolean visitFurtherCodeOwnerConfigs =
codeOwnerConfigVisitor.visit(pathCodeOwners.get().getCodeOwnerConfig());
boolean ignoreParentCodeOwners = pathCodeOwners.get().ignoreParentCodeOwners();
+ if (ignoreParentCodeOwners) {
+ parentCodeOwnersIgnoredCallback.accept(codeOwnerConfigKey);
+ }
logger.atFine().log(
"visitFurtherCodeOwnerConfigs = %s, ignoreParentCodeOwners = %s",
visitFurtherCodeOwnerConfigs, ignoreParentCodeOwners);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
index d05245c..4e2e342 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
@@ -395,6 +395,90 @@
.isEqualTo(CodeOwnerStatus.APPROVED);
}
+ @Test
+ public void notApprovedByFallbackCodeOwnerIfParentCodeOwnersIgnored() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .ignoreParentCodeOwners()
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file is not approved yet.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Add a fallback code owner as reviewer.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
+ // defined).
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Add a Code-Review+1 (= code owner approval) from a fallback code owner.
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId);
+
+ // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
+ // defined).
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void notImplicitlyApprovedByFallbackCodeOwnerIfParentCodeOwnersIgnored() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .ignoreParentCodeOwners()
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file is not approved yet.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
index 2b30264..af08aa3 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
@@ -33,6 +33,7 @@
import com.google.inject.Inject;
import java.io.IOException;
import java.nio.file.Paths;
+import java.util.function.Consumer;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -53,6 +54,7 @@
@Rule public final MockitoRule mockito = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);
@Mock private CodeOwnerConfigVisitor visitor;
+ @Mock private Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback;
@Inject private ProjectOperations projectOperations;
@@ -145,6 +147,22 @@
}
@Test
+ public void cannotVisitCodeOwnerConfigsWithNullCallback() throws Exception {
+ BranchNameKey branchNameKey = BranchNameKey.create(project, "master");
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ codeOwnerConfigHierarchy.visit(
+ branchNameKey,
+ getCurrentRevision(branchNameKey),
+ Paths.get("/foo/bar/baz.md"),
+ visitor,
+ /* parentCodeOwnersIgnoredCallback= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("parentCodeOwnersIgnoredCallback");
+ }
+
+ @Test
public void visitorNotInvokedIfNoCodeOwnerConfigExists() throws Exception {
visit("master", "/foo/bar/baz.md");
verifyZeroInteractions(visitor);
@@ -414,6 +432,9 @@
.verify(visitor)
.visit(codeOwnerConfigOperations.codeOwnerConfig(fooBarCodeOwnerConfigKey).get());
verifyNoMoreInteractions(visitor);
+
+ verify(parentCodeOwnersIgnoredCallback).accept(fooBarCodeOwnerConfigKey);
+ verifyNoMoreInteractions(parentCodeOwnersIgnoredCallback);
}
@Test
@@ -477,6 +498,9 @@
.verify(visitor)
.visit(codeOwnerConfigOperations.codeOwnerConfig(fooBarCodeOwnerConfigKey).get());
verifyNoMoreInteractions(visitor);
+
+ verify(parentCodeOwnersIgnoredCallback).accept(fooBarCodeOwnerConfigKey);
+ verifyNoMoreInteractions(parentCodeOwnersIgnoredCallback);
}
@Test
@@ -533,6 +557,8 @@
.verify(visitor)
.visit(codeOwnerConfigOperations.codeOwnerConfig(rootCodeOwnerConfigKey).get());
verifyNoMoreInteractions(visitor);
+
+ verifyZeroInteractions(parentCodeOwnersIgnoredCallback);
}
@Test
@@ -657,6 +683,9 @@
visit("master", "/foo/bar/baz.md");
verify(visitor).visit(codeOwnerConfigOperations.codeOwnerConfig(rootCodeOwnerConfigKey).get());
verifyNoMoreInteractions(visitor);
+
+ verify(parentCodeOwnersIgnoredCallback).accept(rootCodeOwnerConfigKey);
+ verifyNoMoreInteractions(parentCodeOwnersIgnoredCallback);
}
@Test
@@ -711,7 +740,11 @@
throws InvalidPluginConfigurationException, IOException {
BranchNameKey branchNameKey = BranchNameKey.create(project, branchName);
codeOwnerConfigHierarchy.visit(
- branchNameKey, getCurrentRevision(branchNameKey), Paths.get(path), visitor);
+ branchNameKey,
+ getCurrentRevision(branchNameKey),
+ Paths.get(path),
+ visitor,
+ parentCodeOwnersIgnoredCallback);
}
private ObjectId getCurrentRevision(BranchNameKey branchNameKey) throws IOException {
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 0d2789f..9fd1aa5 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -215,7 +215,9 @@
<a id="pluginCodeOwnersFallbackCodeOwners">plugin.@PLUGIN@.fallbackCodeOwners</a>
: Policy that controls who should own paths that have no code owners
- defined.\
+ defined. This policy only applies if the inheritance of parent code
+ owners hasn't been explicity disabled in a relevant code owner config
+ file.\
\
Can be `NONE` or `ALL_USERS`.\
\
@@ -434,7 +436,9 @@
<a id="codeOwnersFallbackCodeOwners">codeOwners.fallbackCodeOwners</a>
: Policy that controls who should own paths that have no code owners
- defined.\
+ defined. This policy only applies if the inheritance of parent code
+ owners hasn't been explicity disabled in a relevant code owner config
+ file.\
Can be `NONE` or `ALL_USERS` (see
[plugin.@PLUGIN@.fallbackCodeOwners](#pluginCodeOwnersFallbackCodeOwners)
for an explanation of these values).\
