Do not apply fallback code owners if there is a non-resolvable import
Fallback code owners should only apply if there are no code owners
defined for a path, but not if there are no code owners because an
import couldn't be resolved. In the latter case the intention was to
define code owners, and hence the fallback code owners should not apply.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I75ea58af0d28d86390ddbf663e3b32ea182920be
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 7f7cfff..74e2db6 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -153,11 +153,10 @@
.filePath(codeOwnerConfig.key().fileName().orElse("<default>"))
.build())) {
logger.atFine().log("resolving path code owners for path %s", absolutePath);
+ PathCodeOwnersResult pathCodeOwnersResult =
+ pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).resolveCodeOwnerConfig();
return resolve(
- pathCodeOwnersFactory
- .create(codeOwnerConfig, absolutePath)
- .resolveCodeOwnerConfig()
- .getPathCodeOwners());
+ pathCodeOwnersResult.getPathCodeOwners(), pathCodeOwnersResult.hasUnresolvedImports());
}
}
@@ -179,6 +178,19 @@
* @see #resolve(CodeOwnerReference)
*/
public CodeOwnerResolverResult resolve(Set<CodeOwnerReference> codeOwnerReferences) {
+ return resolve(codeOwnerReferences, /* hasUnresolvedImports= */ false);
+ }
+
+ /**
+ * Resolves the given {@link CodeOwnerReference}s to {@link CodeOwner}s.
+ *
+ * @param codeOwnerReferences the code owner references that should be resolved
+ * @param hasUnresolvedImports whether there are unresolved imports
+ * @return the {@link CodeOwner} for the given code owner references
+ * @see #resolve(CodeOwnerReference)
+ */
+ private CodeOwnerResolverResult resolve(
+ Set<CodeOwnerReference> codeOwnerReferences, boolean hasUnresolvedImports) {
requireNonNull(codeOwnerReferences, "codeOwnerReferences");
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
AtomicBoolean hasUnresolvedCodeOwners = new AtomicBoolean(false);
@@ -204,7 +216,7 @@
.map(Optional::get)
.collect(toImmutableSet());
return CodeOwnerResolverResult.create(
- codeOwners, ownedByAllUsers.get(), hasUnresolvedCodeOwners.get());
+ codeOwners, ownedByAllUsers.get(), hasUnresolvedCodeOwners.get(), hasUnresolvedImports);
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
index 4d034c3..a03f0e1 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
@@ -46,12 +46,18 @@
/** Whether there are code owner references which couldn't be resolved. */
public abstract boolean hasUnresolvedCodeOwners();
+ /** Whether there are imports which couldn't be resolved. */
+ public abstract boolean hasUnresolvedImports();
+
/**
* Whether there are any code owners defined for the path, regardless of whether they can be
* resolved or not.
*/
public boolean hasRevelantCodeOwnerDefinitions() {
- return !codeOwners().isEmpty() || ownedByAllUsers() || hasUnresolvedCodeOwners();
+ return !codeOwners().isEmpty()
+ || ownedByAllUsers()
+ || hasUnresolvedCodeOwners()
+ || hasUnresolvedImports();
}
@Override
@@ -60,6 +66,7 @@
.add("codeOwners", codeOwners())
.add("ownedByAllUsers", ownedByAllUsers())
.add("hasUnresolvedCodeOwners", hasUnresolvedCodeOwners())
+ .add("hasUnresolvedImports", hasUnresolvedImports())
.toString();
}
@@ -67,8 +74,9 @@
public static CodeOwnerResolverResult create(
ImmutableSet<CodeOwner> codeOwners,
boolean ownedByAllUsers,
- boolean hasUnresolvedCodeOwners) {
+ boolean hasUnresolvedCodeOwners,
+ boolean hasUnresolvedImports) {
return new AutoValue_CodeOwnerResolverResult(
- codeOwners, ownedByAllUsers, hasUnresolvedCodeOwners);
+ codeOwners, ownedByAllUsers, hasUnresolvedCodeOwners, hasUnresolvedImports);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
index 4e2e342..e087873 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
@@ -479,6 +479,189 @@
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
}
+ @Test
+ public void notApprovedByFallbackCodeOwnerIfImportCannotBeResolved() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addImport(
+ CodeOwnerConfigReference.create(CodeOwnerConfigImportMode.ALL, "/non-existing/OWNERS"))
+ .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 notImplicitlyApprovedByFallbackCodeOwnerIfImportCannotBeResolved() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addImport(
+ CodeOwnerConfigReference.create(CodeOwnerConfigImportMode.ALL, "/non-existing/OWNERS"))
+ .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);
+ }
+
+ @Test
+ public void notApprovedByFallbackCodeOwnerIfPerFileImportCannotBeResolved() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression("*.md")
+ .addImport(
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, "/bar/OWNERS"))
+ .autoBuild())
+ .create();
+
+ Path path = Paths.get("/foo/bar.md");
+ 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 notImplicitlyApprovedByFallbackCodeOwnerIfPerFileImportCannotBeResolved()
+ throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression("*.md")
+ .addImport(
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, "/bar/OWNERS"))
+ .autoBuild())
+ .create();
+
+ Path path = Paths.get("/foo/bar.md");
+ 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/resources/Documentation/config.md b/resources/Documentation/config.md
index 9fd1aa5..6e62b51 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -217,7 +217,7 @@
: Policy that controls who should own paths that have no code owners
defined. This policy only applies if the inheritance of parent code
owners hasn't been explicity disabled in a relevant code owner config
- file.\
+ file and if there are no unresolved imports.\
\
Can be `NONE` or `ALL_USERS`.\
\
@@ -438,7 +438,7 @@
: Policy that controls who should own paths that have no code owners
defined. This policy only applies if the inheritance of parent code
owners hasn't been explicity disabled in a relevant code owner config
- file.\
+ file and if there are no unresolved imports.\
Can be `NONE` or `ALL_USERS` (see
[plugin.@PLUGIN@.fallbackCodeOwners](#pluginCodeOwnersFallbackCodeOwners)
for an explanation of these values).\
