Support configuring multiple override approvals
With this change it is now possible to configure multiple approvals that
count as override for the code owners submit check. If multiple
approvals are configured, any of them is sufficient to override the code
owners submit check.
The configured override label is already returned to clients from the
Get Code Owner Branch Config REST endpoint and the Get Code Owner
Project Config REST endpoint, see the 'override_approval' field in the
respective response entity. Currently the 'override_approval' field
contains a RequiredApprovalInfo entity, but since we support multiple
override labels now we change this to a list of RequiredApprovalInfo
entities. This is an incompatible change that breaks the frontend. To
avoid a breakage the frontend was adapted in change I3677ab4d4 so that
it now understands the old and the new response format. We should submit
this only once that frontend change has been rolled out.
Duplicate override approvals are filtered out and are not returned to
the frontend. As duplicates we consider:
* exact identical override approvals (e.g. "Owners-Override+1" and
"Owners-Override+1")
* override approvals with the same label name and a higher value (e.g.
"Owners-Override+2" is not needed if "Owners-Override+1" is also set,
since "Owners-Override+1" covers all "Owners-Override" approvals >= 1)
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I39e09c27ed4b52f60c460fe1ba1306e7073fe9fa
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
index e06e1bb..f944314 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
@@ -187,16 +187,20 @@
}
protected void createOwnersOverrideLabel() throws RestApiException {
+ createOwnersOverrideLabel("Owners-Override");
+ }
+
+ protected void createOwnersOverrideLabel(String labelName) throws RestApiException {
LabelDefinitionInput input = new LabelDefinitionInput();
input.values = ImmutableMap.of("+1", "Override", " 0", "No Override");
- gApi.projects().name(project.get()).label("Owners-Override").create(input).get();
+ gApi.projects().name(project.get()).label(labelName).create(input).get();
// Allow to vote on the Owners-Override label.
projectOperations
.project(project)
.forUpdate()
.add(
- TestProjectUpdate.allowLabel("Owners-Override")
+ TestProjectUpdate.allowLabel(labelName)
.range(0, 1)
.ref("refs/*")
.group(REGISTERED_USERS)
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerBranchConfigInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerBranchConfigInfo.java
index c0ae04b..6fb3fc7 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerBranchConfigInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerBranchConfigInfo.java
@@ -13,6 +13,8 @@
// limitations under the License.
package com.google.gerrit.plugins.codeowners.api;
+import java.util.List;
+
/**
* Representation of the code owner branch configuration in the REST API.
*
@@ -53,11 +55,14 @@
public RequiredApprovalInfo requiredApproval;
/**
- * The approval that is required to override the code owners submit check.
+ * The approvals that count as override for the code owners submit check.
+ *
+ * <p>If multiple approvals are returned, any of them is sufficient to override the code owners
+ * submit check.
*
* <p>Not set if {@link #disabled} is {@code true}.
*/
- public RequiredApprovalInfo overrideApproval;
+ public List<RequiredApprovalInfo> overrideApproval;
/**
* Whether the branch doesn't contain any code owner config file yet.
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInfo.java
index f559739..bb9e1df 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInfo.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.codeowners.api;
+import java.util.List;
+
/**
* Representation of the code owner project configuration in the REST API.
*
@@ -52,9 +54,12 @@
public RequiredApprovalInfo requiredApproval;
/**
- * The approval that is required to override the code owners submit check.
+ * The approval that count as override for the code owners submit check.
+ *
+ * <p>If multiple approvals are returned, any of them is sufficient to override the code owners
+ * submit check.
*
* <p>Not set if {@code status.disabled} is {@code true}.
*/
- public RequiredApprovalInfo overrideApproval;
+ public List<RequiredApprovalInfo> overrideApproval;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index dafbbbf..8211b7c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -174,12 +174,11 @@
codeOwnersPluginConfiguration.getRequiredApproval(changeNotes.getProjectName());
logger.atFine().log("requiredApproval = %s", requiredApproval);
- Optional<RequiredApproval> overrideApproval =
+ ImmutableSet<RequiredApproval> overrideApprovals =
codeOwnersPluginConfiguration.getOverrideApproval(changeNotes.getProjectName());
- boolean hasOverride =
- overrideApproval.isPresent() && hasOverride(overrideApproval.get(), changeNotes);
+ boolean hasOverride = hasOverride(overrideApprovals, changeNotes);
logger.atFine().log(
- "hasOverride = %s (overrideApproval = %s)", hasOverride, overrideApproval);
+ "hasOverride = %s (overrideApprovals = %s)", hasOverride, overrideApprovals);
BranchNameKey branch = changeNotes.getChange().getDest();
ObjectId revision = getDestBranchRevision(changeNotes.getChange());
@@ -638,13 +637,17 @@
/**
* Checks whether the given change has an override approval.
*
- * @param overrideApproval approval that is required to override the code owners submit check.
+ * @param overrideApprovals approvals that count as override for the code owners submit check.
* @param changeNotes the change notes
* @return whether the given change has an override approval
*/
- private boolean hasOverride(RequiredApproval overrideApproval, ChangeNotes changeNotes) {
+ private boolean hasOverride(
+ ImmutableSet<RequiredApproval> overrideApprovals, ChangeNotes changeNotes) {
return changeNotes.getApprovals().get(changeNotes.getCurrentPatchSet().id()).stream()
- .anyMatch(overrideApproval::isApprovedBy);
+ .anyMatch(
+ patchSetApproval ->
+ overrideApprovals.stream()
+ .anyMatch(overrideApproval -> overrideApproval.isApprovedBy(patchSetApproval)));
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
index 9f088a7..a66dc1b 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
@@ -36,6 +36,8 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.util.HashMap;
+import java.util.Map;
import java.util.Optional;
import org.eclipse.jgit.lib.Config;
@@ -348,8 +350,11 @@
}
/**
- * Returns the approval that is required to override the code owners submit check for a change of
- * the given project.
+ * Returns the approvals that are required to override the code owners submit check for a change
+ * of the given project.
+ *
+ * <p>If multiple approvals are returned, any of them is sufficient to override the code owners
+ * submit check.
*
* <p>Callers must ensure that the project of the specified branch exists. If the project doesn't
* exist the call fails with {@link IllegalStateException}.
@@ -363,22 +368,14 @@
*
* <p>The first override approval configuration that exists counts and the evaluation is stopped.
*
- * <p>If the code owner configuration contains multiple override values, the last value is used.
- *
* @param project project for which the override approval should be returned
- * @return the override approval that should be used for the given project, {@link
- * Optional#empty()} if no override approval is configured, in this case the override
- * functionality is disabled
+ * @return the override approvals that should be used for the given project, an empty set if no
+ * override approval is configured, in this case the override functionality is disabled
*/
- public Optional<RequiredApproval> getOverrideApproval(Project.NameKey project) {
+ public ImmutableSet<RequiredApproval> getOverrideApproval(Project.NameKey project) {
try {
- ImmutableList<RequiredApproval> configuredOverrideApprovalConfig =
- getConfiguredRequiredApproval(overrideApprovalConfig, project);
- if (!configuredOverrideApprovalConfig.isEmpty()) {
- // There can be only one override approval. If multiple ones are configured just use the
- // last one, this is also what Config#getString(String, String, String) does.
- return Optional.of(Iterables.getLast(configuredOverrideApprovalConfig));
- }
+ return filterOutDuplicateRequiredApprovals(
+ getConfiguredRequiredApproval(overrideApprovalConfig, project));
} catch (InvalidPluginConfigurationException e) {
logger.atWarning().withCause(e).log(
"Ignoring invalid override approval configuration for project %s."
@@ -386,7 +383,34 @@
project.get());
}
- return Optional.empty();
+ return ImmutableSet.of();
+ }
+
+ /**
+ * Filters out duplicate required approvals from the input list.
+ *
+ * <p>The following entries are considered as duplicate:
+ *
+ * <ul>
+ * <li>exact identical required approvals (e.g. "Code-Review+2" and "Code-Review+2")
+ * <li>required approvals with the same label name and a higher value (e.g. "Code-Review+2" is
+ * not needed if "Code-Review+1" is already contained, since "Code-Review+1" covers all
+ * "Code-Review" approvals >= 1)
+ * </ul>
+ */
+ private ImmutableSet<RequiredApproval> filterOutDuplicateRequiredApprovals(
+ ImmutableList<RequiredApproval> requiredApprovals) {
+ Map<String, RequiredApproval> requiredApprovalsByLabel = new HashMap<>();
+ for (RequiredApproval requiredApproval : requiredApprovals) {
+ String labelName = requiredApproval.labelType().getName();
+ RequiredApproval otherRequiredApproval = requiredApprovalsByLabel.get(labelName);
+ if (otherRequiredApproval != null
+ && otherRequiredApproval.value() <= requiredApproval.value()) {
+ continue;
+ }
+ requiredApprovalsByLabel.put(labelName, requiredApproval);
+ }
+ return ImmutableSet.copyOf(requiredApprovalsByLabel.values());
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
index 65d93e8..e7fcd72 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
@@ -157,12 +157,14 @@
return formatRequiredApproval(codeOwnersPluginConfiguration.getRequiredApproval(projectName));
}
+ @VisibleForTesting
@Nullable
- private RequiredApprovalInfo formatOverrideApprovalInfo(Project.NameKey projectName) {
- return codeOwnersPluginConfiguration
- .getOverrideApproval(projectName)
- .map(CodeOwnerProjectConfigJson::formatRequiredApproval)
- .orElse(null);
+ ImmutableList<RequiredApprovalInfo> formatOverrideApprovalInfo(Project.NameKey projectName) {
+ ImmutableList<RequiredApprovalInfo> overrideApprovalInfos =
+ codeOwnersPluginConfiguration.getOverrideApproval(projectName).stream()
+ .map(CodeOwnerProjectConfigJson::formatRequiredApproval)
+ .collect(toImmutableList());
+ return overrideApprovalInfos.isEmpty() ? null : overrideApprovalInfos;
}
@VisibleForTesting
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/RequiredApprovalSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/RequiredApprovalSubject.java
index 27d2b20..68673df 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/RequiredApprovalSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/RequiredApprovalSubject.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertAbout;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.IntegerSubject;
import com.google.common.truth.StringSubject;
@@ -61,6 +62,17 @@
}
/**
+ * Starts a fluent chain to do assertions on a set of {@link RequiredApproval}s.
+ *
+ * @param requiredApprovals set of required approvals on which assertions should be done
+ * @return the created {@link ListSubject}
+ */
+ public static ListSubject<RequiredApprovalSubject, RequiredApproval> assertThat(
+ ImmutableSet<RequiredApproval> requiredApprovals) {
+ return ListSubject.assertThat(requiredApprovals.asList(), requiredApprovals());
+ }
+
+ /**
* Creates a subject factory for mapping {@link RequiredApproval}s to {@link
* RequiredApprovalSubject}s.
*/
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
index daf08e2..19f9a40 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.plugins.codeowners.testing.RequiredApprovalSubject.assertThat;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
@@ -33,7 +34,6 @@
import com.google.gerrit.plugins.codeowners.config.RequiredApproval;
import com.google.gerrit.plugins.codeowners.config.RequiredApprovalConfig;
import com.google.gerrit.plugins.codeowners.config.StatusConfig;
-import java.util.Optional;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.PushResult;
@@ -309,11 +309,11 @@
PushResult r = pushRefsMetaConfig();
assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus()).isEqualTo(Status.OK);
- Optional<RequiredApproval> overrideApproval =
+ ImmutableSet<RequiredApproval> overrideApproval =
codeOwnersPluginConfiguration.getOverrideApproval(project);
- assertThat(overrideApproval).isPresent();
- assertThat(overrideApproval).value().hasLabelNameThat().isEqualTo("Code-Review");
- assertThat(overrideApproval).value().hasValueThat().isEqualTo(2);
+ assertThat(overrideApproval).hasSize(1);
+ assertThat(overrideApproval).element(0).hasLabelNameThat().isEqualTo("Code-Review");
+ assertThat(overrideApproval).element(0).hasValueThat().isEqualTo(2);
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java
index 37d0d23..66edb89 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Project;
@@ -209,8 +210,27 @@
configureOverrideApproval(project, "Code-Review+2");
CodeOwnerBranchConfigInfo codeOwnerBranchConfigInfo =
projectCodeOwnersApiFactory.project(project).branch("master").getConfig();
- assertThat(codeOwnerBranchConfigInfo.overrideApproval.label).isEqualTo("Code-Review");
- assertThat(codeOwnerBranchConfigInfo.overrideApproval.value).isEqualTo(2);
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval).hasSize(1);
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(0).label).isEqualTo("Code-Review");
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(0).value).isEqualTo(2);
+ }
+
+ @Test
+ public void getConfigWithMultipleConfiguredOverrideApproval() throws Exception {
+ createOwnersOverrideLabel();
+ setCodeOwnersConfig(
+ project,
+ null,
+ OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
+ ImmutableList.of("Owners-Override+1", "Code-Review+2"));
+ CodeOwnerBranchConfigInfo codeOwnerBranchConfigInfo =
+ projectCodeOwnersApiFactory.project(project).branch("master").getConfig();
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval).hasSize(2);
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(0).label)
+ .isEqualTo("Owners-Override");
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(0).value).isEqualTo(1);
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(1).label).isEqualTo("Code-Review");
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(1).value).isEqualTo(2);
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java
index e936dda..de208e9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java
@@ -19,6 +19,7 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.Nullable;
@@ -236,8 +237,27 @@
configureOverrideApproval(project, "Code-Review+2");
CodeOwnerProjectConfigInfo codeOwnerProjectConfigInfo =
projectCodeOwnersApiFactory.project(project).getConfig();
- assertThat(codeOwnerProjectConfigInfo.overrideApproval.label).isEqualTo("Code-Review");
- assertThat(codeOwnerProjectConfigInfo.overrideApproval.value).isEqualTo(2);
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval).hasSize(1);
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(0).label).isEqualTo("Code-Review");
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(0).value).isEqualTo(2);
+ }
+
+ @Test
+ public void getConfigWithMultipleConfiguredOverrideApproval() throws Exception {
+ createOwnersOverrideLabel();
+ setCodeOwnersConfig(
+ project,
+ null,
+ OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
+ ImmutableList.of("Owners-Override+1", "Code-Review+2"));
+ CodeOwnerProjectConfigInfo codeOwnerProjectConfigInfo =
+ projectCodeOwnersApiFactory.project(project).getConfig();
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval).hasSize(2);
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(0).label)
+ .isEqualTo("Owners-Override");
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(0).value).isEqualTo(1);
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(1).label).isEqualTo("Code-Review");
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(1).value).isEqualTo(2);
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index da58fca..12bd0f2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -1536,6 +1536,89 @@
}
@Test
+ @GerritConfig(
+ name = "plugin.code-owners.overrideApproval",
+ values = {"Owners-Override+1", "Another-Override+1"})
+ public void getStatus_anyOverrideApprovesAllFiles() throws Exception {
+ // create arbitrary code owner config to avoid entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+ createArbitraryCodeOwnerConfigFile();
+
+ createOwnersOverrideLabel();
+ createOwnersOverrideLabel("Another-Override");
+
+ // Create a change.
+ String changeId =
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ImmutableMap.of(
+ "foo/baz.config", "content",
+ "bar/baz.config", "other content"))
+ .to("refs/for/master")
+ .getChangeId();
+
+ // Without override approval the expected status is INSUFFICIENT_REVIEWERS.
+ for (FileCodeOwnerStatus fileCodeOwnerStatus :
+ codeOwnerApprovalCheck
+ .getFileStatuses(getChangeNotes(changeId))
+ .collect(toImmutableList())) {
+ assertThat(fileCodeOwnerStatus)
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ // Add an override approval.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
+
+ // With override approval the expected status is APPROVED.
+ for (FileCodeOwnerStatus fileCodeOwnerStatus :
+ codeOwnerApprovalCheck
+ .getFileStatuses(getChangeNotes(changeId))
+ .collect(toImmutableList())) {
+ assertThat(fileCodeOwnerStatus)
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ // Delete the override approval.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0));
+
+ // Without override approval the expected status is INSUFFICIENT_REVIEWERS.
+ for (FileCodeOwnerStatus fileCodeOwnerStatus :
+ codeOwnerApprovalCheck
+ .getFileStatuses(getChangeNotes(changeId))
+ .collect(toImmutableList())) {
+ assertThat(fileCodeOwnerStatus)
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ // Add another override approval.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Another-Override", 1));
+
+ // With override approval the expected status is APPROVED.
+ for (FileCodeOwnerStatus fileCodeOwnerStatus :
+ codeOwnerApprovalCheck
+ .getFileStatuses(getChangeNotes(changeId))
+ .collect(toImmutableList())) {
+ assertThat(fileCodeOwnerStatus)
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+ }
+
+ @Test
public void cannotCheckIfSubmittableForNullChangeNotes() throws Exception {
NullPointerException npe =
assertThrows(
@@ -1624,6 +1707,53 @@
}
@Test
+ @GerritConfig(
+ name = "plugin.code-owners.overrideApproval",
+ values = {"Owners-Override+1", "Another-Override+1"})
+ public void isSubmittableIfAnyOverrideIsPresent() throws Exception {
+ // create arbitrary code owner config to avoid entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+ createArbitraryCodeOwnerConfigFile();
+
+ createOwnersOverrideLabel();
+ createOwnersOverrideLabel("Another-Override");
+
+ // Create a change.
+ String changeId =
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ImmutableMap.of(
+ "foo/baz.config", "content",
+ "bar/baz.config", "other content"))
+ .to("refs/for/master")
+ .getChangeId();
+
+ // Without override approval the change is not submittable.
+ assertThat(codeOwnerApprovalCheck.isSubmittable(getChangeNotes(changeId))).isFalse();
+
+ // Add an override approval.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
+
+ // With override approval the change is submittable.
+ assertThat(codeOwnerApprovalCheck.isSubmittable(getChangeNotes(changeId))).isTrue();
+
+ // Delete the override approval.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0));
+
+ // Without override approval the change is not submittable.
+ assertThat(codeOwnerApprovalCheck.isSubmittable(getChangeNotes(changeId))).isFalse();
+
+ // Add another override approval.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Another-Override", 1));
+
+ // With override approval the change is submittable.
+ assertThat(codeOwnerApprovalCheck.isSubmittable(getChangeNotes(changeId))).isTrue();
+ }
+
+ @Test
public void bootstrappingGetStatus_insufficientReviewers() throws Exception {
// since no code owner config exists we are entering the bootstrapping code path in
// CodeOwnerApprovalCheck
@@ -1850,6 +1980,90 @@
}
@Test
+ @GerritConfig(
+ name = "plugin.code-owners.overrideApproval",
+ values = {"Owners-Override+1", "Another-Override+1"})
+ public void bootstrappingGetStatus_anyOverrideApprovesAllFiles() throws Exception {
+ // since no code owner config exists we are entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+
+ createOwnersOverrideLabel();
+ createOwnersOverrideLabel("Another-Override");
+
+ // Create a change with a user that is not a project owner.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project, user);
+ String changeId =
+ pushFactory
+ .create(
+ user.newIdent(),
+ testRepo,
+ "Test Change",
+ ImmutableMap.of(
+ "foo/baz.config", "content",
+ "bar/baz.config", "other content"))
+ .to("refs/for/master")
+ .getChangeId();
+
+ // Without override approval the expected status is INSUFFICIENT_REVIEWERS.
+ for (FileCodeOwnerStatus fileCodeOwnerStatus :
+ codeOwnerApprovalCheck
+ .getFileStatuses(getChangeNotes(changeId))
+ .collect(toImmutableList())) {
+ assertThat(fileCodeOwnerStatus)
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ // Add an override approval (by a user that is not a project owners, and hence no code owner).
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
+
+ // With override approval the expected status is APPROVED.
+ for (FileCodeOwnerStatus fileCodeOwnerStatus :
+ codeOwnerApprovalCheck
+ .getFileStatuses(getChangeNotes(changeId))
+ .collect(toImmutableList())) {
+ assertThat(fileCodeOwnerStatus)
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ // Delete the override approval.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0));
+
+ // Without override approval the expected status is INSUFFICIENT_REVIEWERS.
+ for (FileCodeOwnerStatus fileCodeOwnerStatus :
+ codeOwnerApprovalCheck
+ .getFileStatuses(getChangeNotes(changeId))
+ .collect(toImmutableList())) {
+ assertThat(fileCodeOwnerStatus)
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ // Add another override approval.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Another-Override", 1));
+
+ // With override approval the expected status is APPROVED.
+ for (FileCodeOwnerStatus fileCodeOwnerStatus :
+ codeOwnerApprovalCheck
+ .getFileStatuses(getChangeNotes(changeId))
+ .collect(toImmutableList())) {
+ assertThat(fileCodeOwnerStatus)
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+ }
+
+ @Test
public void getStatus_branchDeleted() throws Exception {
String branchName = "tempBranch";
createBranch(BranchNameKey.create(project, branchName));
diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
index dab2b0d..832e491 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.truth.OptionalSubject.assertThat;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.Nullable;
@@ -672,37 +673,42 @@
@Test
@GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Code-Review+2")
public void getConfiguredDefaultOverrideApproval() throws Exception {
- Optional<RequiredApproval> requiredApproval =
+ ImmutableSet<RequiredApproval> requiredApproval =
codeOwnersPluginConfiguration.getOverrideApproval(project);
- assertThat(requiredApproval).isPresent();
- assertThat(requiredApproval).value().hasLabelNameThat().isEqualTo("Code-Review");
- assertThat(requiredApproval).value().hasValueThat().isEqualTo(2);
+ assertThat(requiredApproval).hasSize(1);
+ assertThat(requiredApproval).element(0).hasLabelNameThat().isEqualTo("Code-Review");
+ assertThat(requiredApproval).element(0).hasValueThat().isEqualTo(2);
}
@Test
public void getOverrideApprovalConfiguredOnProjectLevel() throws Exception {
configureOverrideApproval(project, "Code-Review+2");
- Optional<RequiredApproval> requiredApproval =
+ ImmutableSet<RequiredApproval> requiredApproval =
codeOwnersPluginConfiguration.getOverrideApproval(project);
- assertThat(requiredApproval).isPresent();
- assertThat(requiredApproval).value().hasLabelNameThat().isEqualTo("Code-Review");
- assertThat(requiredApproval).value().hasValueThat().isEqualTo(2);
+ assertThat(requiredApproval).hasSize(1);
+ assertThat(requiredApproval).element(0).hasLabelNameThat().isEqualTo("Code-Review");
+ assertThat(requiredApproval).element(0).hasValueThat().isEqualTo(2);
}
@Test
public void getOverrideApprovalMultipleConfiguredOnProjectLevel() throws Exception {
+ createOwnersOverrideLabel();
+ createOwnersOverrideLabel("Other-Override");
+
setCodeOwnersConfig(
project,
/* subsection= */ null,
OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
- ImmutableList.of("Code-Review+2", "Code-Review+1"));
+ ImmutableList.of("Owners-Override+1", "Other-Override+1"));
// If multiple values are set for a key, the last value wins.
- Optional<RequiredApproval> requiredApproval =
+ ImmutableSet<RequiredApproval> requiredApproval =
codeOwnersPluginConfiguration.getOverrideApproval(project);
- assertThat(requiredApproval).isPresent();
- assertThat(requiredApproval).value().hasLabelNameThat().isEqualTo("Code-Review");
- assertThat(requiredApproval).value().hasValueThat().isEqualTo(1);
+ assertThat(requiredApproval).hasSize(2);
+ assertThat(requiredApproval).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
+ assertThat(requiredApproval).element(0).hasValueThat().isEqualTo(1);
+ assertThat(requiredApproval).element(1).hasLabelNameThat().isEqualTo("Other-Override");
+ assertThat(requiredApproval).element(1).hasValueThat().isEqualTo(1);
}
@Test
@@ -719,6 +725,22 @@
}
@Test
+ public void getOverrideApprovalDuplicatesAreFilteredOut() throws Exception {
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
+ ImmutableList.of("Code-Review+2", "Code-Review+1", "Code-Review+2"));
+
+ // If multiple values are set for a key, the last value wins.
+ ImmutableSet<RequiredApproval> requiredApproval =
+ codeOwnersPluginConfiguration.getOverrideApproval(project);
+ assertThat(requiredApproval).hasSize(1);
+ assertThat(requiredApproval).element(0).hasLabelNameThat().isEqualTo("Code-Review");
+ assertThat(requiredApproval).element(0).hasValueThat().isEqualTo(1);
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.enableExperimentalRestEndpoints", value = "false")
public void checkExperimentalRestEndpointsEnabledThrowsExceptionIfDisabled() throws Exception {
MethodNotAllowedException exception =
diff --git a/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java b/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
index 30f8454..45d957a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
@@ -21,6 +21,8 @@
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
@@ -165,7 +167,7 @@
.thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2));
when(codeOwnersPluginConfiguration.getOverrideApproval(project))
.thenReturn(
- Optional.of(
+ ImmutableSet.of(
RequiredApproval.create(
LabelType.withDefaultValues("Owners-Override"), (short) 1)));
@@ -185,8 +187,10 @@
.containsExactly("refs/heads/stable-2.10", CodeOwnerBackendId.PROTO.getBackendId());
assertThat(codeOwnerProjectConfigInfo.requiredApproval.label).isEqualTo("Code-Review");
assertThat(codeOwnerProjectConfigInfo.requiredApproval.value).isEqualTo(2);
- assertThat(codeOwnerProjectConfigInfo.overrideApproval.label).isEqualTo("Owners-Override");
- assertThat(codeOwnerProjectConfigInfo.overrideApproval.value).isEqualTo(1);
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval).hasSize(1);
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(0).label)
+ .isEqualTo("Owners-Override");
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(0).value).isEqualTo(1);
}
@Test
@@ -227,6 +231,25 @@
}
@Test
+ public void withMultipleOverrides() throws Exception {
+ createOwnersOverrideLabel();
+
+ when(codeOwnersPluginConfiguration.getOverrideApproval(project))
+ .thenReturn(
+ ImmutableSet.of(
+ RequiredApproval.create(LabelType.withDefaultValues("Owners-Override"), (short) 1),
+ RequiredApproval.create(LabelType.withDefaultValues("Code-Review"), (short) 2)));
+
+ ImmutableList<RequiredApprovalInfo> requiredApprovalInfos =
+ codeOwnerProjectConfigJson.formatOverrideApprovalInfo(project);
+ assertThat(requiredApprovalInfos).hasSize(2);
+ assertThat(requiredApprovalInfos.get(0).label).isEqualTo("Owners-Override");
+ assertThat(requiredApprovalInfos.get(0).value).isEqualTo(1);
+ assertThat(requiredApprovalInfos.get(1).label).isEqualTo("Code-Review");
+ assertThat(requiredApprovalInfos.get(1).value).isEqualTo(2);
+ }
+
+ @Test
public void formatCodeOwnerBranchConfig() throws Exception {
createOwnersOverrideLabel();
@@ -251,7 +274,7 @@
.thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2));
when(codeOwnersPluginConfiguration.getOverrideApproval(project))
.thenReturn(
- Optional.of(
+ ImmutableSet.of(
RequiredApproval.create(
LabelType.withDefaultValues("Owners-Override"), (short) 1)));
@@ -268,8 +291,10 @@
.isEqualTo(CodeOwnerBackendId.FIND_OWNERS.getBackendId());
assertThat(codeOwnerBranchConfigInfo.requiredApproval.label).isEqualTo("Code-Review");
assertThat(codeOwnerBranchConfigInfo.requiredApproval.value).isEqualTo(2);
- assertThat(codeOwnerBranchConfigInfo.overrideApproval.label).isEqualTo("Owners-Override");
- assertThat(codeOwnerBranchConfigInfo.overrideApproval.value).isEqualTo(1);
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval).hasSize(1);
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(0).label)
+ .isEqualTo("Owners-Override");
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(0).value).isEqualTo(1);
assertThat(codeOwnerBranchConfigInfo.noCodeOwnersDefined).isNull();
}
@@ -304,7 +329,7 @@
.thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2));
when(codeOwnersPluginConfiguration.getOverrideApproval(project))
.thenReturn(
- Optional.of(
+ ImmutableSet.of(
RequiredApproval.create(
LabelType.withDefaultValues("Owners-Override"), (short) 1)));
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index b67693b..c7258e0 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -151,10 +151,13 @@
By default "Code-Review+1".
<a id="pluginCodeOwnersOverrideApproval">plugin.@PLUGIN@.overrideApproval</a>
-: Approval that is required to override the code owners submit check.\
+: Approval that counts as override for the code owners submit check.\
The override approval must be specified in the format
"\<label-name\>+\<label-value\>".\
- The configured label must exist for all projects for which this setting
+ Can be specifed multiple times to configure multiple override approvals.
+ If multiple approvals are configured, any of them is sufficient to
+ override the code owners submit check.\
+ The configured labels must exist for all projects for which this setting
applies (all projects that have code owners enabled and for which this
setting is not overridden).\
Can be overridden per project by setting
@@ -379,10 +382,13 @@
`gerrit.config` is used.
<a id="codeOwnersOverrideApproval">codeOwners.overrideApproval</a>
-: Approval that is required to override the code owners submit check.\
+: Approval that counts as override for the code owners submit check.\
The override approval must be specified in the format
"\<label-name\>+\<label-value\>".\
- The configured label must exist for all projects for which this setting
+ Can be specifed multiple times to configure multiple override approvals.
+ If multiple approvals are configured, any of them is sufficient to
+ override the code owners submit check.\
+ The configured labels must exist for all projects for which this setting
applies (all projects that have code owners enabled and for which this
setting is not overridden).\
Overrides the global setting
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 3fd9f84..b79b178 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -54,10 +54,12 @@
"label": "Code-Review",
"value": 1
},
- "override_approval": {
- "label": "Owners-Override",
- "value": 1
- }
+ "override_approval": [
+ {
+ "label": "Owners-Override",
+ "value": 1
+ }
+ ]
}
```
@@ -161,10 +163,12 @@
"label": "Code-Review",
"value": 1
},
- "override_approval": {
- "label": "Owners-Override",
- "value": 1
- }
+ "override_approval": [
+ {
+ "label": "Owners-Override",
+ "value": 1
+ }
+ ]
}
```
@@ -612,7 +616,7 @@
| `disabled` | optional | Whether the code owners functionality is disabled for the branch. If `true` the code owners API is disabled and submitting changes doesn't require code owner approvals. Not set if `false`.
| `backend_id`| optional | ID of the code owner backend that is configured for the branch. Not set if `disabled` is `true`.
| `required_approval` | optional | The approval that is required from code owners to approve the files in a change as [RequiredApprovalInfo](#required-approval-info) entity. The required approval defines which approval counts as code owner approval. Not set if `disabled` is `true`.
-| `override_approval` | optional | The approval that is required to override the code owners submit check as [RequiredApprovalInfo](#required-approval-info) entity. If unset, overriding the code owners submit check is disabled. Not set if `disabled` is `true`.
+| `override_approval` | optional | Approvals that count as override for the code owners submit check as a list of [RequiredApprovalInfo](#required-approval-info) entities. If multiple approvals are returned, any of them is sufficient to override the code owners submit check. All returned override approvals are guarenteed to have distinct label names. If unset, overriding the code owners submit check is disabled. Not set if `disabled` is `true`.
| `no_code_owners_defined` | optional | Whether the branch doesn't contain any code owner config file yet. If a branch doesn't contain any code owner config file yet, the projects owners are considered as code owners. Once a first code owner config file is added to the branch, the project owners are no longer code owners (unless code ownership is granted to them via the code owner config file). Not set if `false` or if `disabled` is `true`.
---
@@ -627,7 +631,7 @@
| `status` | optional | The code owner status configuration as [CodeOwnersStatusInfo](#code-owners-status-info) entity. Contains information about whether the code owners functionality is disabled for the project or for any branch.
| `backend` | optional | The code owner backend configuration as [BackendInfo](#backend-info) entity. Not set if `status.disabled` is `true`.
| `required_approval` | optional | The approval that is required from code owners to approve the files in a change as [RequiredApprovalInfo](#required-approval-info) entity. The required approval defines which approval counts as code owner approval. Not set if `status.disabled` is `true`.
-| `override_approval` | optional | The approval that is required to override the code owners submit check as [RequiredApprovalInfo](#required-approval-info) entity. If unset, overriding the code owners submit check is disabled. Not set if `status.disabled` is `true`.
+| `override_approval` | optional | Approvals that count as override for the code owners submit check as a list of [RequiredApprovalInfo](#required-approval-info) entities. If multiple approvals are returned, any of them is sufficient to override the code owners submit check. All returned override approvals are guarenteed to have distinct label names. If unset, overriding the code owners submit check is disabled. Not set if `disabled` is `true`.
---
