Merge "Config guide: Document security pitfalls"
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/BranchCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwners.java
index 82fb351..22bc543 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwners.java
@@ -34,6 +34,7 @@
abstract class CodeOwnerConfigFilesRequest {
private boolean includeNonParsableFiles;
private String email;
+ private String path;
/** Includes non-parsable code owner config files into the result. */
public CodeOwnerConfigFilesRequest includeNonParsableFiles(boolean includeNonParsableFiles) {
@@ -62,6 +63,23 @@
return email;
}
+ /**
+ * Limits the returned code owner config files to those that have a path matching the given
+ * glob.
+ *
+ * @param path the path glob that should be matched
+ */
+ public CodeOwnerConfigFilesRequest withPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /** Returns the path glob that should be matched by the returned code owner config files/ */
+ @Nullable
+ public String getPath() {
+ return path;
+ }
+
/** Executes the request and retrieves the paths of the requested code owner config file */
public abstract List<String> paths() throws RestApiException;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwnersImpl.java b/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwnersImpl.java
index f6e4804..7052a1b 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwnersImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwnersImpl.java
@@ -66,6 +66,7 @@
GetCodeOwnerConfigFiles getCodeOwnerConfigFiles = getCodeOwnerConfigFilesProvider.get();
getCodeOwnerConfigFiles.setIncludeNonParsableFiles(getIncludeNonParsableFiles());
getCodeOwnerConfigFiles.setEmail(getEmail());
+ getCodeOwnerConfigFiles.setPath(getPath());
return getCodeOwnerConfigFiles.apply(branchResource).value();
}
};
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 f868c42..26d67f0 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -175,12 +175,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());
@@ -740,13 +739,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/backend/findowners/FindOwnersCodeOwnerConfigParser.java b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
index 561c3b1..7a4261a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
@@ -50,14 +50,6 @@
* <p>The syntax is described at in the {@code find-owners} plugin documentation at:
* https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/resources/Documentation/syntax.md
*
- * <p><strong>Note:</strong> Currently this class only supports a subset of the syntax. Only the
- * following syntax elements are supported:
- *
- * <ul>
- * <li>comment: a line can be a comment (comments must start with '#')
- * <li>code owner emails: a line can be the email of a code owner
- * </ul>
- *
* <p>Comment lines are silently ignored.
*
* <p>Invalid lines cause the parsing to fail and trigger a {@link CodeOwnerConfigParseException}.
diff --git a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigValidator.java
index 2a29766..8f20fc1 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigValidator.java
@@ -14,8 +14,6 @@
package com.google.gerrit.plugins.codeowners.config;
-import static com.google.gerrit.server.project.ProjectCache.illegalState;
-
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Project;
@@ -30,7 +28,7 @@
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.ValidationMessage;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.ProjectLevelConfig;
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
@@ -41,6 +39,7 @@
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
/** Validates modifications to the {@code code-owners.config} file in {@code refs/meta/config}. */
@Singleton
@@ -48,8 +47,9 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final String pluginName;
- private final ProjectCache projectCache;
private final GitRepositoryManager repoManager;
+ private final ProjectConfig.Factory projectConfigFactory;
+ private final ProjectState.Factory projectStateFactory;
private final ChangedFiles changedFiles;
private final GeneralConfig generalConfig;
private final StatusConfig statusConfig;
@@ -60,8 +60,9 @@
@Inject
CodeOwnersPluginConfigValidator(
@PluginName String pluginName,
- ProjectCache projectCache,
GitRepositoryManager repoManager,
+ ProjectConfig.Factory projectConfigFactory,
+ ProjectState.Factory projectStateFactory,
ChangedFiles changedFiles,
GeneralConfig generalConfig,
StatusConfig statusConfig,
@@ -69,8 +70,9 @@
RequiredApprovalConfig requiredApprovalConfig,
OverrideApprovalConfig overrideApprovalConfig) {
this.pluginName = pluginName;
- this.projectCache = projectCache;
this.repoManager = repoManager;
+ this.projectConfigFactory = projectConfigFactory;
+ this.projectStateFactory = projectStateFactory;
this.changedFiles = changedFiles;
this.generalConfig = generalConfig;
this.statusConfig = statusConfig;
@@ -93,11 +95,11 @@
return ImmutableList.of();
}
- ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
+ ProjectState projectState = getProjectState(project, receiveEvent.commit);
ProjectLevelConfig.Bare cfg = loadConfig(project, fileName, receiveEvent.commit);
validateConfig(projectState, fileName, cfg);
return ImmutableList.of();
- } catch (IOException | PatchListNotAvailableException e) {
+ } catch (IOException | ConfigInvalidException | PatchListNotAvailableException e) {
String errorMessage =
String.format(
"failed to validate file %s for revision %s in ref %s of project %s",
@@ -107,6 +109,15 @@
}
}
+ private ProjectState getProjectState(Project.NameKey projectName, RevCommit commit)
+ throws IOException, ConfigInvalidException {
+ try (Repository repo = repoManager.openRepository(projectName)) {
+ ProjectConfig projectConfig = projectConfigFactory.create(projectName);
+ projectConfig.load(repo, commit);
+ return projectStateFactory.create(projectConfig.getCacheable());
+ }
+ }
+
/**
* Whether the given file was changed in the given revision.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
index fe9017b..57bc488 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
@@ -37,6 +37,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;
@@ -360,8 +362,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}.
@@ -375,22 +380,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."
@@ -398,7 +395,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/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java
index 54ddf7c..8a15dcb 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java
@@ -227,7 +227,7 @@
+ " plugin.%s.%s). Falling back to default value %s.",
pluginConfigFromGerritConfig.getString(KEY_FALLBACK_CODE_OWNERS),
pluginName,
- KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED,
+ KEY_FALLBACK_CODE_OWNERS,
FallbackCodeOwners.NONE);
return FallbackCodeOwners.NONE;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
index d034419..bf3a8ad 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
@@ -16,6 +16,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
@@ -159,12 +160,15 @@
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()
+ .sorted(comparing(requiredApproval -> requiredApproval.toString()))
+ .map(CodeOwnerProjectConfigJson::formatRequiredApproval)
+ .collect(toImmutableList());
+ return overrideApprovalInfos.isEmpty() ? null : overrideApprovalInfos;
}
@VisibleForTesting
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerConfigFiles.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerConfigFiles.java
index 8190e7c..9ec7118 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerConfigFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerConfigFiles.java
@@ -51,6 +51,7 @@
private boolean includeNonParsableFiles;
private String email;
+ private String pathGlob;
@Option(
name = "--include-non-parsable-files",
@@ -67,6 +68,15 @@
this.email = email;
}
+ @Option(
+ name = "--path",
+ usage =
+ "limits the returned code owner config files to those that have a path matching"
+ + " this glob")
+ public void setPath(@Nullable String pathGlob) {
+ this.pathGlob = pathGlob;
+ }
+
@Inject
public GetCodeOwnerConfigFiles(
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
@@ -108,7 +118,8 @@
? (codeOwnerConfigFilePath, configInvalidException) -> {
codeOwnerConfigs.add(codeOwnerConfigFilePath);
}
- : CodeOwnerConfigScanner.ignoreInvalidCodeOwnerConfigFiles());
+ : CodeOwnerConfigScanner.ignoreInvalidCodeOwnerConfigFiles(),
+ pathGlob);
return Response.ok(
codeOwnerConfigs.build().stream().map(Path::toString).collect(toImmutableList()));
}
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 610a6b1..1ea4183 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;
@@ -34,11 +35,12 @@
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.revwalk.RevObject;
import org.eclipse.jgit.transport.PushResult;
import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
+import org.eclipse.jgit.util.RawParseUtils;
import org.junit.Before;
import org.junit.Test;
@@ -310,11 +312,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
@@ -369,6 +371,52 @@
}
@Test
+ public void defineAndConfigureOverrideLabelInSameCommit() throws Exception {
+ fetchRefsMetaConfig();
+
+ RevCommit head = getHead(testRepo.getRepository(), RefNames.REFS_CONFIG);
+ RevObject blob = testRepo.get(head.getTree(), "project.config");
+ byte[] data = testRepo.getRepository().open(blob).getCachedBytes(Integer.MAX_VALUE);
+ String projectConfigText = RawParseUtils.decode(data);
+
+ Config projectConfig = new Config();
+ projectConfig.fromText(projectConfigText);
+ String labelName = "Owners-Override";
+ projectConfig.setString("label", labelName, "function", "NoOp");
+ projectConfig.setStringList(
+ "label", labelName, "value", ImmutableList.of("0 Not Override", "+1 Override"));
+
+ Config codeOwnersConfig = new Config();
+ codeOwnersConfig.setString(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
+ "Owners-Override+1");
+
+ RevCommit commit =
+ testRepo.update(
+ RefNames.REFS_CONFIG,
+ testRepo
+ .commit()
+ .parent(head)
+ .message("Add test code owner config")
+ .author(admin.newIdent())
+ .committer(admin.newIdent())
+ .add("code-owners.config", codeOwnersConfig.toText())
+ .add("project.config", projectConfig.toText()));
+
+ testRepo.reset(commit);
+
+ PushResult r = pushRefsMetaConfig();
+ assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus()).isEqualTo(Status.OK);
+ ImmutableSet<RequiredApproval> overrideApproval =
+ codeOwnersPluginConfiguration.getOverrideApproval(project);
+ assertThat(overrideApproval).hasSize(1);
+ assertThat(overrideApproval).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
+ assertThat(overrideApproval).element(0).hasValueThat().isEqualTo(1);
+ }
+
+ @Test
public void configureMergeCommitStrategy() throws Exception {
fetchRefsMetaConfig();
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 7b2dae6..a838795 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java
@@ -17,10 +17,10 @@
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;
-import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -31,16 +31,10 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.config.BackendConfig;
-import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.config.GeneralConfig;
import com.google.gerrit.plugins.codeowners.config.OverrideApprovalConfig;
import com.google.gerrit.plugins.codeowners.config.RequiredApprovalConfig;
import com.google.gerrit.plugins.codeowners.config.StatusConfig;
-import org.eclipse.jgit.junit.TestRepository;
-import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -221,8 +215,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,
+ /* subsection= */ 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("Code-Review");
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(0).value).isEqualTo(2);
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(1).label)
+ .isEqualTo("Owners-Override");
+ assertThat(codeOwnerBranchConfigInfo.overrideApproval.get(1).value).isEqualTo(1);
}
@Test
@@ -243,70 +256,70 @@
private void configureFileExtension(Project.NameKey project, String fileExtension)
throws Exception {
- setConfig(project, null, GeneralConfig.KEY_FILE_EXTENSION, fileExtension);
+ setCodeOwnersConfig(
+ project, /* subsection= */ null, GeneralConfig.KEY_FILE_EXTENSION, fileExtension);
}
private void configureOverrideInfoUrl(Project.NameKey project, String overrideInfoUrl)
throws Exception {
- setConfig(project, null, GeneralConfig.KEY_OVERRIDE_INFO_URL, overrideInfoUrl);
+ setCodeOwnersConfig(
+ project, /* subsection= */ null, GeneralConfig.KEY_OVERRIDE_INFO_URL, overrideInfoUrl);
}
private void configureMergeCommitStrategy(
Project.NameKey project, MergeCommitStrategy mergeCommitStrategy) throws Exception {
- setConfig(project, null, GeneralConfig.KEY_MERGE_COMMIT_STRATEGY, mergeCommitStrategy.name());
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ GeneralConfig.KEY_MERGE_COMMIT_STRATEGY,
+ mergeCommitStrategy.name());
}
private void configureFallbackCodeOwners(
Project.NameKey project, FallbackCodeOwners fallbackCodeOwners) throws Exception {
- setConfig(project, null, GeneralConfig.KEY_FALLBACK_CODE_OWNERS, fallbackCodeOwners.name());
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ fallbackCodeOwners.name());
}
private void configureDisabledBranch(Project.NameKey project, String disabledBranch)
throws Exception {
- setCodeOwnersConfig(project, null, StatusConfig.KEY_DISABLED_BRANCH, disabledBranch);
+ setCodeOwnersConfig(
+ project, /* subsection= */ null, StatusConfig.KEY_DISABLED_BRANCH, disabledBranch);
}
private void configureBackend(Project.NameKey project, String backendName) throws Exception {
- configureBackend(project, null, backendName);
+ configureBackend(project, /* branch= */ null, backendName);
}
private void configureBackend(
Project.NameKey project, @Nullable String branch, String backendName) throws Exception {
- setConfig(project, branch, BackendConfig.KEY_BACKEND, backendName);
+ setCodeOwnersConfig(project, branch, BackendConfig.KEY_BACKEND, backendName);
}
private void configureRequiredApproval(Project.NameKey project, String requiredApproval)
throws Exception {
- setConfig(project, null, RequiredApprovalConfig.KEY_REQUIRED_APPROVAL, requiredApproval);
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ RequiredApprovalConfig.KEY_REQUIRED_APPROVAL,
+ requiredApproval);
}
private void configureOverrideApproval(Project.NameKey project, String overrideApproval)
throws Exception {
- setConfig(project, null, OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL, overrideApproval);
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
+ overrideApproval);
}
private void configureImplicitApprovals(Project.NameKey project) throws Exception {
- setConfig(project, null, GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS, "true");
- }
-
- private void setConfig(Project.NameKey project, String subsection, String key, String value)
- throws Exception {
- Config codeOwnersConfig = new Config();
- codeOwnersConfig.setString(
- CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS, subsection, key, value);
- try (TestRepository<Repository> testRepo =
- new TestRepository<>(repoManager.openRepository(project))) {
- Ref ref = testRepo.getRepository().exactRef(RefNames.REFS_CONFIG);
- RevCommit head = testRepo.getRevWalk().parseCommit(ref.getObjectId());
- testRepo.update(
- RefNames.REFS_CONFIG,
- testRepo
- .commit()
- .parent(head)
- .message("Configure code owner backend")
- .add("code-owners.config", codeOwnersConfig.toText()));
- }
- projectCache.evict(project);
+ setCodeOwnersConfig(
+ project, /* subsection= */ null, GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS, "true");
}
/** Returns the ID of a code owner backend that is not the given backend. */
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigFilesIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigFilesIT.java
index 284f01b..31d1b87 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigFilesIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigFilesIT.java
@@ -373,6 +373,68 @@
.isEmpty();
}
+ @Test
+ public void getCodeOwnerConfigFilesWithPathGlob() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ CodeOwnerConfig.Key codeOwnerConfigKey2 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ CodeOwnerConfig.Key codeOwnerConfigKey3 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ assertThat(
+ projectCodeOwnersApiFactory
+ .project(project)
+ .branch("master")
+ .codeOwnerConfigFiles()
+ .withPath("/foo/bar/" + getCodeOwnerConfigFileName())
+ .paths())
+ .containsExactly(
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey3).getFilePath());
+
+ assertThat(
+ projectCodeOwnersApiFactory
+ .project(project)
+ .branch("master")
+ .codeOwnerConfigFiles()
+ .withPath("/foo/bar/*")
+ .paths())
+ .containsExactly(
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey3).getFilePath())
+ .inOrder();
+
+ assertThat(
+ projectCodeOwnersApiFactory
+ .project(project)
+ .branch("master")
+ .codeOwnerConfigFiles()
+ .withPath("/foo/**")
+ .paths())
+ .containsExactly(
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getFilePath(),
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey3).getFilePath())
+ .inOrder();
+ }
+
private String getCodeOwnerConfigFileName() {
CodeOwnerBackend backend = backendConfig.getDefaultBackend();
if (backend instanceof FindOwnersBackend) {
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 4f61669..fe6c044 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java
@@ -19,12 +19,12 @@
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;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -35,17 +35,11 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.config.BackendConfig;
-import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.config.GeneralConfig;
import com.google.gerrit.plugins.codeowners.config.OverrideApprovalConfig;
import com.google.gerrit.plugins.codeowners.config.RequiredApprovalConfig;
import com.google.gerrit.plugins.codeowners.config.StatusConfig;
import com.google.inject.Inject;
-import org.eclipse.jgit.junit.TestRepository;
-import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -248,8 +242,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,
+ /* subsection= */ 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("Code-Review");
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(0).value).isEqualTo(2);
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(1).label)
+ .isEqualTo("Owners-Override");
+ assertThat(codeOwnerProjectConfigInfo.overrideApproval.get(1).value).isEqualTo(1);
}
@Test
@@ -262,70 +275,70 @@
private void configureFileExtension(Project.NameKey project, String fileExtension)
throws Exception {
- setConfig(project, null, GeneralConfig.KEY_FILE_EXTENSION, fileExtension);
+ setCodeOwnersConfig(
+ project, /* subsection= */ null, GeneralConfig.KEY_FILE_EXTENSION, fileExtension);
}
private void configureOverrideInfoUrl(Project.NameKey project, String overrideInfoUrl)
throws Exception {
- setConfig(project, null, GeneralConfig.KEY_OVERRIDE_INFO_URL, overrideInfoUrl);
+ setCodeOwnersConfig(
+ project, /* subsection= */ null, GeneralConfig.KEY_OVERRIDE_INFO_URL, overrideInfoUrl);
}
private void configureMergeCommitStrategy(
Project.NameKey project, MergeCommitStrategy mergeCommitStrategy) throws Exception {
- setConfig(project, null, GeneralConfig.KEY_MERGE_COMMIT_STRATEGY, mergeCommitStrategy.name());
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ GeneralConfig.KEY_MERGE_COMMIT_STRATEGY,
+ mergeCommitStrategy.name());
}
private void configureFallbackCodeOwners(
Project.NameKey project, FallbackCodeOwners fallbackCodeOwners) throws Exception {
- setConfig(project, null, GeneralConfig.KEY_FALLBACK_CODE_OWNERS, fallbackCodeOwners.name());
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ fallbackCodeOwners.name());
}
private void configureDisabledBranch(Project.NameKey project, String disabledBranch)
throws Exception {
- setCodeOwnersConfig(project, null, StatusConfig.KEY_DISABLED_BRANCH, disabledBranch);
+ setCodeOwnersConfig(
+ project, /* subsection= */ null, StatusConfig.KEY_DISABLED_BRANCH, disabledBranch);
}
private void configureBackend(Project.NameKey project, String backendName) throws Exception {
- configureBackend(project, null, backendName);
+ configureBackend(project, /* branch= */ null, backendName);
}
private void configureBackend(
Project.NameKey project, @Nullable String branch, String backendName) throws Exception {
- setConfig(project, branch, BackendConfig.KEY_BACKEND, backendName);
+ setCodeOwnersConfig(project, branch, BackendConfig.KEY_BACKEND, backendName);
}
private void configureRequiredApproval(Project.NameKey project, String requiredApproval)
throws Exception {
- setConfig(project, null, RequiredApprovalConfig.KEY_REQUIRED_APPROVAL, requiredApproval);
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ RequiredApprovalConfig.KEY_REQUIRED_APPROVAL,
+ requiredApproval);
}
private void configureOverrideApproval(Project.NameKey project, String overrideApproval)
throws Exception {
- setConfig(project, null, OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL, overrideApproval);
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
+ overrideApproval);
}
private void configureImplicitApprovals(Project.NameKey project) throws Exception {
- setConfig(project, null, GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS, "true");
- }
-
- private void setConfig(Project.NameKey project, String subsection, String key, String value)
- throws Exception {
- Config codeOwnersConfig = new Config();
- codeOwnersConfig.setString(
- CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS, subsection, key, value);
- try (TestRepository<Repository> testRepo =
- new TestRepository<>(repoManager.openRepository(project))) {
- Ref ref = testRepo.getRepository().exactRef(RefNames.REFS_CONFIG);
- RevCommit head = testRepo.getRevWalk().parseCommit(ref.getObjectId());
- testRepo.update(
- RefNames.REFS_CONFIG,
- testRepo
- .commit()
- .parent(head)
- .message("Configure code owner backend")
- .add("code-owners.config", codeOwnersConfig.toText()));
- }
- projectCache.evict(project);
+ setCodeOwnersConfig(
+ project, /* subsection= */ null, GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS, "true");
}
/** Returns the ID of a code owner backend that is not the given backend. */
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 84626e4..6386a15 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -27,12 +27,14 @@
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
+import com.google.gerrit.extensions.common.LabelDefinitionInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.JgitPath;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
@@ -1539,6 +1541,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(
@@ -1627,6 +1712,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
@@ -1853,6 +1985,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));
@@ -2002,6 +2218,70 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void ownersOverridePlus2CountsAsOverrideIfOverridePlus1IsRequired() throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.values = ImmutableMap.of("+2", "Override+2", "+1", "Override", " 0", "No Override");
+ gApi.projects().name(project.get()).label("Owners-Override").create(input).get();
+
+ // Allow to vote on the Owners-Override label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Owners-Override")
+ .range(0, 2)
+ .ref("refs/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ TestAccount user2 = accountCreator.user2();
+
+ // Create a code owner config file with 'admin' as code owner
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ // Create a change as 'user' that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "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);
+
+ // Let 'user2' override with Owners-Override+2
+ requestScopeOperations.setApiUser(user2.id());
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 2));
+
+ // Check that the file is approved now.
+ requestScopeOperations.setApiUser(admin.id());
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @Test
public void noBootstrappingIfDefaultCodeOwnerConfigExists() throws Exception {
TestAccount user2 = accountCreator.user2();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReferenceTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReferenceTest.java
index 747bd83..42d3696 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReferenceTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReferenceTest.java
@@ -14,10 +14,15 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.truth.PathSubject.paths;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import java.util.Optional;
import org.junit.Test;
@@ -45,4 +50,23 @@
.build());
assertThat(exception).hasMessageThat().isEqualTo("branch must be full name: master");
}
+
+ @Test
+ public void absoluteFilePathCanBeSpecifiedInDifferentFormats() throws Exception {
+ Path expectedPath = Paths.get("/foo/OWNERS");
+ for (String inputPath : new String[] {"/foo/OWNERS", "//foo/OWNERS"}) {
+ Path path =
+ CodeOwnerConfigReference.create(CodeOwnerConfigImportMode.ALL, inputPath).filePath();
+ assertWithMessage(inputPath).about(paths()).that(path).isEqualTo(expectedPath);
+ assertThat(path.isAbsolute()).isTrue();
+ }
+ }
+
+ @Test
+ public void relativeFilePathCanBeSpecified() throws Exception {
+ Path path =
+ CodeOwnerConfigReference.create(CodeOwnerConfigImportMode.ALL, "foo/OWNERS").filePath();
+ assertThat(path).isEqualTo(Paths.get("foo/OWNERS"));
+ assertThat(path.isAbsolute()).isFalse();
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
index 0677ca4..066fcbb 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
@@ -81,7 +81,9 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> pathCodeOwnersFactory.create(null, Paths.get("/foo/bar/baz.md")));
+ () ->
+ pathCodeOwnersFactory.create(
+ /* codeOwnerConfig= */ null, Paths.get("/foo/bar/baz.md")));
assertThat(npe).hasMessageThat().isEqualTo("codeOwnerConfig");
}
@@ -90,7 +92,8 @@
CodeOwnerConfig codeOwnerConfig = createCodeOwnerBuilder().build();
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> pathCodeOwnersFactory.create(codeOwnerConfig, null));
+ NullPointerException.class,
+ () -> pathCodeOwnersFactory.create(codeOwnerConfig, /* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("path");
}
@@ -133,7 +136,7 @@
NullPointerException.class,
() ->
pathCodeOwnersFactory.create(
- null,
+ /* codeOwnerConfigKey= */ null,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md")));
assertThat(npe).hasMessageThat().isEqualTo("codeOwnerConfigKey");
@@ -148,7 +151,7 @@
pathCodeOwnersFactory.create(
CodeOwnerConfig.Key.create(
BranchNameKey.create(project, "master"), Paths.get("/")),
- null,
+ /* revision= */ null,
Paths.get("/foo/bar/baz.md")));
assertThat(npe).hasMessageThat().isEqualTo("revision");
}
@@ -171,7 +174,7 @@
pathCodeOwnersFactory.create(
codeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
- null));
+ /* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("path");
}
@@ -265,7 +268,7 @@
@GerritConfig(name = "plugin.code-owners.backend", value = TestCodeOwnerBackend.ID)
public void codeOwnerSetsWithPathExpressionsAreIgnoredIfBackendDoesntSupportPathExpressions()
throws Exception {
- try (AutoCloseable registration = registerTestBackend(null)) {
+ try (AutoCloseable registration = registerTestBackend(/* pathExpressionMatcher= */ null)) {
CodeOwnerConfig codeOwnerConfig =
createCodeOwnerBuilder()
.addCodeOwnerSet(
@@ -1664,7 +1667,9 @@
NullPointerException.class,
() ->
PathCodeOwners.matches(
- null, Paths.get("bar/baz.md"), mock(PathExpressionMatcher.class)));
+ /* codeOwnerSet= */ null,
+ Paths.get("bar/baz.md"),
+ mock(PathExpressionMatcher.class)));
assertThat(npe).hasMessageThat().isEqualTo("codeOwnerSet");
}
@@ -1676,7 +1681,7 @@
() ->
PathCodeOwners.matches(
CodeOwnerSet.createWithoutPathExpressions(admin.email()),
- null,
+ /* relativePath= */ null,
mock(PathExpressionMatcher.class)));
assertThat(npe).hasMessageThat().isEqualTo("relativePath");
}
@@ -1706,7 +1711,7 @@
PathCodeOwners.matches(
CodeOwnerSet.createWithoutPathExpressions(admin.email()),
Paths.get("bar/baz.md"),
- null));
+ /* matcher= */ null));
assertThat(npe).hasMessageThat().isEqualTo("matcher");
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
index d40fba6..6acb268 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
@@ -14,12 +14,14 @@
package com.google.gerrit.plugins.codeowners.config;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.RequiredApprovalSubject.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
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;
@@ -673,37 +675,41 @@
@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> requiredApprovals =
codeOwnersPluginConfiguration.getOverrideApproval(project);
- assertThat(requiredApproval).isPresent();
- assertThat(requiredApproval).value().hasLabelNameThat().isEqualTo("Code-Review");
- assertThat(requiredApproval).value().hasValueThat().isEqualTo(1);
+ assertThat(
+ requiredApprovals.stream()
+ .map(requiredApproval -> requiredApproval.toString())
+ .collect(toImmutableSet()))
+ .containsExactly("Owners-Override+1", "Other-Override+1");
}
@Test
@@ -720,6 +726,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 deba4b3..51b6ea5 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;
@@ -168,7 +170,7 @@
.thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2));
when(codeOwnersPluginConfiguration.getOverrideApproval(project))
.thenReturn(
- Optional.of(
+ ImmutableSet.of(
RequiredApproval.create(
LabelType.withDefaultValues("Owners-Override"), (short) 1)));
@@ -190,8 +192,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
@@ -232,6 +236,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("Code-Review");
+ assertThat(requiredApprovalInfos.get(0).value).isEqualTo(2);
+ assertThat(requiredApprovalInfos.get(1).label).isEqualTo("Owners-Override");
+ assertThat(requiredApprovalInfos.get(1).value).isEqualTo(1);
+ }
+
+ @Test
public void formatCodeOwnerBranchConfig() throws Exception {
createOwnersOverrideLabel();
@@ -258,7 +281,7 @@
.thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2));
when(codeOwnersPluginConfiguration.getOverrideApproval(project))
.thenReturn(
- Optional.of(
+ ImmutableSet.of(
RequiredApproval.create(
LabelType.withDefaultValues("Owners-Override"), (short) 1)));
@@ -277,8 +300,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();
}
@@ -315,7 +340,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 1b972db..b01532a 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -139,6 +139,8 @@
<a id="pluginCodeOwnersRequiredApproval">plugin.@PLUGIN@.requiredApproval</a>
: Approval that is required from code owners to approve the files in a
change.\
+ Any approval on the configured label that has a value >= the configured
+ value is considered as code owner approval.\
The required approval must be specified in the format
"\<label-name\>+\<label-value\>".\
The configured label must exist for all projects for which this setting
@@ -154,10 +156,15 @@
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.\
+ Any approval on the configured label that has a value >= the configured
+ value is considered as code owner override.\
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
@@ -394,6 +401,8 @@
<a id="codeOwnersRequiredApproval">codeOwners.requiredApproval</a>
: Approval that is required from code owners to approve the files in a
change.\
+ Any approval on the configured label that has a value >= the configured
+ value is considered as code owner approval.\
The required approval must be specified in the format
"\<label-name\>+\<label-value\>".\
The configured label must exist for all projects for which this setting
@@ -411,10 +420,15 @@
`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.\
+ Any approval on the configured label that has a value >= the configured
+ value is considered as code owner override.\
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 80a58d9..bd25b37 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
+ }
+ ]
}
```
@@ -184,6 +188,7 @@
| ----------- | -------- | ----------- |
| `include-non-parsable-files` | optional | Includes non-parseable code owner config files in the response. By default `false`. Cannot be used in combination with the `email` option.
| `email` | optional | Code owner email that must appear in the returned code owner config files.
+| `path` | optional | Path glob that must be matched by the returned code owner config files.
#### Request
@@ -612,8 +617,8 @@
| `general` | optional | The general code owners configuration as [GeneralInfo](#general-info) entity. Not set if `disabled` is `true`.
| `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`.
+| `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. Any approval on this label with a value >= the given value is considered as code owner approval. 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 (sorted alphabetically). 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. Any approval on these labels with a value >= the given values is considered as code owner override. 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`.
---
@@ -628,7 +633,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`.
---
