Merge "Improve a11y of code owners status column"
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
index eda1ee9..bab4ac7 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
@@ -19,7 +19,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Streams;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -155,9 +154,7 @@
info.path = ownedPath.path().toString();
info.owned = ownedPath.owned() ? true : null;
info.owners =
- Streams.stream(ownedPath.owners())
- .map(GetOwnedPaths::toAccountInfo)
- .collect(toImmutableList());
+ ownedPath.owners().stream().map(GetOwnedPaths::toAccountInfo).collect(toImmutableList());
return info;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/OwnedChangedFileInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/OwnedChangedFileInfoSubject.java
index eb6a76e..10276d6 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/OwnedChangedFileInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/OwnedChangedFileInfoSubject.java
@@ -63,11 +63,13 @@
}
public void hasOwnedNewPath(String expectedOwnedNewPath) {
+ @SuppressWarnings("unused")
var unused = hasNewPathThat(expectedOwnedNewPath);
check("ownedNewPath").that(ownedChangedFileInfo().newPath.owned).isTrue();
}
public void hasNonOwnedNewPath(String expectedNonOwnedNewPath) {
+ @SuppressWarnings("unused")
var unused = hasNewPathThat(expectedNonOwnedNewPath);
check("ownedNewPath").that(ownedChangedFileInfo().newPath.owned).isNull();
}
@@ -77,11 +79,13 @@
}
public void hasOwnedOldPath(String expectedOwnedOldPath) {
+ @SuppressWarnings("unused")
var unused = hasOldPathThat(expectedOwnedOldPath);
check("ownedOldPath").that(ownedChangedFileInfo().oldPath.owned).isTrue();
}
public void hasNonOwnedOldPath(String expectedNonOwnedOldPath) {
+ @SuppressWarnings("unused")
var unused = hasOldPathThat(expectedNonOwnedOldPath);
check("ownedOldPath").that(ownedChangedFileInfo().oldPath.owned).isNull();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
index aa1d939..3c41f14 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
@@ -18,6 +18,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
+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.request.RequestScopeOperations;
@@ -254,6 +255,120 @@
assertThat(loadInput().checkAllOwners()).isFalse();
}
+ @Test
+ public void createInputForComputingOwnedPaths_noReviewers() throws Exception {
+ changeApi().addReviewer(user.email());
+ changeApi().addReviewer(user2.email());
+
+ // CodeOwnerApprovalCheckInput#createForComputingOwnedPaths never sets reviewers
+ assertThat(createInputForComputingOwnedPaths(ImmutableSet.of()).reviewers()).isEmpty();
+ }
+
+ @Test
+ public void createInputForComputingOwnedPaths_noApprovers() throws Exception {
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // CodeOwnerApprovalCheckInput#createForComputingOwnedPaths always sets approvers to the given
+ // accounts
+ assertThat(createInputForComputingOwnedPaths(ImmutableSet.of()).approvers()).isEmpty();
+ }
+
+ @Test
+ public void createInputForComputingOwnedPaths_withApprovers() {
+ // CodeOwnerApprovalCheckInput#createForComputingOwnedPaths always sets approvers to the given
+ // accounts
+ assertThat(
+ createInputForComputingOwnedPaths(ImmutableSet.of(admin.id(), user.id(), user2.id()))
+ .approvers())
+ .containsExactly(admin.id(), user.id(), user2.id());
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void createInputForComputingOwnedPaths_noImplicitApprover() {
+ // CodeOwnerApprovalCheckInput#createForComputingOwnedPaths never sets an implicit approver
+ assertThat(createInputForComputingOwnedPaths(ImmutableSet.of()).implicitApprover()).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void createInputForComputingOwnedPaths_noOverride() throws Exception {
+ createOwnersOverrideLabel();
+
+ ReviewInput reviewInput = new ReviewInput().label("Owners-Override", 1);
+
+ // self override
+ requestScopeOperations.setApiUser(changeOwner);
+ gApi.changes().id(changeId.toString()).current().review(reviewInput);
+
+ // override as user
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(changeId.toString()).current().review(reviewInput);
+
+ // override as user2
+ requestScopeOperations.setApiUser(user2.id());
+ gApi.changes().id(changeId.toString()).current().review(reviewInput);
+
+ // CodeOwnerApprovalCheckInput#createForComputingOwnedPaths never sets overrides
+ assertThat(createInputForComputingOwnedPaths(ImmutableSet.of()).overrides()).isEmpty();
+ }
+
+ @Test
+ public void createInputForComputingOwnedPaths_noGlobalCodeOwners() {
+ CodeOwnerResolverResult globalCodeOwners =
+ createInputForComputingOwnedPaths(ImmutableSet.of()).globalCodeOwners();
+ assertThat(globalCodeOwners.codeOwnersAccountIds()).isEmpty();
+ assertThat(globalCodeOwners.ownedByAllUsers()).isFalse();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.globalCodeOwner",
+ values = {"user1@example.com", "user2@example.com"})
+ public void createInputForComputingOwnedPaths_withGlobalCodeOwners() {
+ CodeOwnerResolverResult globalCodeOwners =
+ createInputForComputingOwnedPaths(ImmutableSet.of()).globalCodeOwners();
+ assertThat(globalCodeOwners.codeOwnersAccountIds()).containsExactly(user.id(), user2.id());
+ assertThat(globalCodeOwners.ownedByAllUsers()).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void createInputForComputingOwnedPaths_withAllUsersAsGlobalCodeOwners() {
+ CodeOwnerResolverResult globalCodeOwners =
+ createInputForComputingOwnedPaths(ImmutableSet.of()).globalCodeOwners();
+ assertThat(globalCodeOwners.codeOwnersAccountIds()).isEmpty();
+ assertThat(globalCodeOwners.ownedByAllUsers()).isTrue();
+ }
+
+ @Test
+ public void createInputForComputingOwnedPaths_withFallbackCodeOwnersNone() {
+ assertThat(createInputForComputingOwnedPaths(ImmutableSet.of()).fallbackCodeOwners())
+ .isEqualTo(FallbackCodeOwners.NONE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ public void createInputForComputingOwnedPaths_withFallbackCodeOwnersAllUsers() {
+ assertThat(createInputForComputingOwnedPaths(ImmutableSet.of()).fallbackCodeOwners())
+ .isEqualTo(FallbackCodeOwners.ALL_USERS);
+ }
+
+ @Test
+ public void createInputForComputingOwnedPaths_checkAllOwnersIsTrue() {
+ assertThat(createInputForComputingOwnedPaths(ImmutableSet.of()).checkAllOwners()).isTrue();
+ }
+
private void disableSelfCodeReviewApprovals() throws Exception {
disableSelfApprovals(allProjects, "Code-Review");
}
@@ -276,4 +391,11 @@
ChangeNotes changeNotes = changeNotesFactory.create(project, changeId);
return inputLoaderFactory.create(codeOwnersConfig, codeOwnerResolver, changeNotes).load();
}
+
+ private CodeOwnerApprovalCheckInput createInputForComputingOwnedPaths(
+ ImmutableSet<Account.Id> accounts) {
+ ChangeNotes changeNotes = changeNotesFactory.create(project, changeId);
+ return CodeOwnerApprovalCheckInput.createForComputingOwnedPaths(
+ codeOwnersConfig, codeOwnerResolver, changeNotes, accounts);
+ }
}
diff --git a/resources/Documentation/backend-find-owners.md b/resources/Documentation/backend-find-owners.md
index 223cebb..846e8d9 100644
--- a/resources/Documentation/backend-find-owners.md
+++ b/resources/Documentation/backend-find-owners.md
@@ -20,6 +20,21 @@
details about code owner config files are described
[here](backends.html#codeOwnerConfigFiles)).
+**NOTE:** Tools should never parse `OWNERS` files on their own, but instead
+always use the [REST API](rest-api.html) to access code owner information.
+
+Parsing `OWNERS` files is highly discouraged because:
+
+* it's hard for tools to implement the parsing of `OWNERS` files in a way that
+ is consistent with how Gerrit parses them
+* reimplementing the parsing logic outside of Gerrit is a lot of effort that can
+ be saved by using the REST API and letting Gerrit do the parsing
+* parsing `OWNERS` files outside of Gerrit is likely less efficient since the
+ parser likely needs to reach out to Gerrit to fetch additional input (e.g. if
+ `OWNERS` files from other branches or repositories are imported)
+* the format of `OWNERS` files may change (e.g. by the addition of new features)
+ which could break parsers outside of Gerrit
+
### <a id="defaultCodeOwnerConfiguration">Default code owners
Default code owners that apply to all branches can be defined in an `OWNERS`
file in the root directory of the `refs/meta/config` branch. This `OWNERS` file
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index bc4e0d6..0185851 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -914,7 +914,7 @@
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.
+| `debug_logs` | List of debug logs that may help to understand why the user is or isn't a code owner. This information is purely for debugging and the output may be changed at any time. This means bot callers must not parse the debug logs.
---
@@ -1042,7 +1042,7 @@
| ------------- | -------- | ----------- |
| `code_owners` | | List of code owners as [CodeOwnerInfo](#code-owner-info) entities. The code owners are sorted by a score that is computed from mutliple [scoring factors](#scoringFactors).
| `owned_by_all_users` | optional | Whether the path is owned by all users. Not set if `false`.
-| `debug_logs` | optional | Debug logs that may help to understand why a user is or isn't suggested as a code owner. Only set if requested via `--debug`.
+| `debug_logs` | optional | Debug logs that may help to understand why a user is or isn't suggested as a code owner. Only set if requested via `--debug`. This information is purely for debugging and the output may be changed at any time. This means bot callers must not parse the debug logs.
### <a id="file-code-owner-status-info"> FileCodeOwnerStatusInfo
The `FileCodeOwnerStatusInfo` entity describes the code owner statuses for a
