Merge "CodeOwnerApprovalCheck: Allow to check for an account which paths it owns"
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/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/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/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/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
index 93504a7..1ea4183 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -37,8 +37,10 @@
import com.google.gerrit.plugins.codeowners.config.StatusConfig;
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;
@@ -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 d6bf9ab..a838795 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java
@@ -21,7 +21,6 @@
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;
@@ -32,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;
@@ -232,7 +225,7 @@
createOwnersOverrideLabel();
setCodeOwnersConfig(
project,
- null,
+ /* subsection= */ null,
OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
ImmutableList.of("Owners-Override+1", "Code-Review+2"));
CodeOwnerBranchConfigInfo codeOwnerBranchConfigInfo =
@@ -263,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 0a14b8e..fe6c044 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java
@@ -25,7 +25,6 @@
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;
@@ -36,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;
@@ -259,7 +252,7 @@
createOwnersOverrideLabel();
setCodeOwnersConfig(
project,
- null,
+ /* subsection= */ null,
OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
ImmutableList.of("Owners-Override+1", "Code-Review+2"));
CodeOwnerProjectConfigInfo codeOwnerProjectConfigInfo =
@@ -282,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/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 5b6e80b..6acb268 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
@@ -14,6 +14,7 @@
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;
@@ -702,14 +703,13 @@
OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL,
ImmutableList.of("Owners-Override+1", "Other-Override+1"));
- // If multiple values are set for a key, the last value wins.
- ImmutableSet<RequiredApproval> requiredApproval =
+ ImmutableSet<RequiredApproval> requiredApprovals =
codeOwnersPluginConfiguration.getOverrideApproval(project);
- 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);
+ assertThat(
+ requiredApprovals.stream()
+ .map(requiredApproval -> requiredApproval.toString())
+ .collect(toImmutableSet()))
+ .containsExactly("Owners-Override+1", "Other-Override+1");
}
@Test
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
new file mode 100644
index 0000000..0a6904d
--- /dev/null
+++ b/resources/Documentation/config-guide.md
@@ -0,0 +1,125 @@
+# Config Guide
+
+The `@PLUGIN@` plugin has many configuration parameters that can be used to
+customize its behavior. These configuration parameters are described in the
+[config](config.html) documentation. This guide gives some additional
+recommendations for the configuration, but doesn't cover all configuration
+parameters.
+
+## <a id="requiredConfiguration">Required Configuration
+
+**Before** installing/enabling the plugin, or enabling the code owners
+functionality for further projects, it is important to do some basic
+configuration. This includes choosing a [code owner backend](backends.html),
+defining the approvals that count as code owner approval and as code owner
+override, opting-out projects or branches and configuring the allowed email
+domain. All this configuration is covered in detail by the [setup
+guide](setup-guide.html).
+
+## <a id="workflow">Workflow Configuration
+
+Some of the configuration parameters have an effect on the user workflow.
+
+### <a id="implicitApprovals">Implicit code owner approvals
+
+It's possible to [enable implicit approvals](config.html#pluginCodeOwnersEnableImplicitApprovals)
+of code owners on their own changes. If enabled and the uploader of a patch set
+is a code owner, an approval of the uploader is assumed for all owned files.
+This means if a code owner uploads a change / patch set that only touches files
+that they own, no approval from other code owners is required for submitting the
+change.
+
+If implicit approvals are enabled, paths can be exempted from requiring code
+owner approvals by assigning the code ownership to [all
+users](backend-find-owners.html#allUsers), as then any modification to the path
+is always implicitly approved by the uploader.
+
+**NOTE:** If implicit approvals are disabled, users can still self-approve their
+own changes by voting on the required label.
+
+**IMPORTANT**: Enabling implicit approvals is considered unsafe, see [security
+pitfalls](#securityImplicitApprovals) below.
+
+### <a id="mergeCommits">Required code owner approvals on merge commits
+
+For merge commits the list of modified files depends on the base against which
+the merge commit is compared:
+
+1. comparison against the destination branch (aka first parent commit):
+ All files which differ between the merge commit and the destination branch.
+ This includes all files which have been modified in the source branch since
+ the last merge into the destination branch has been done.
+
+2. comparison against the Auto-Merge (Auto-Merge = result of automatically
+ merging the source branch into the destination branch):
+ Only shows files for which a conflict resolution has been done.
+
+Which files a users sees on the change screen depends on their base selection.
+
+For the `@PLUGIN@` plugin it can be configured [which files of a merge commit
+require code owner approvals](config.html#pluginCodeOwnersMergeCommitStrategy),
+all files that differ with the destination branch (case 1) or only files that
+differ with the Auto-Merge (case 2). If case 1 is configured, all file diffs
+that have been approved in one branch must be re-approved when they are merged
+into another branch. If case 2 is configured, only conflict resolutions have to
+be approved when a merge is done.
+
+**IMPORTANT**: Requiring code owner approvals only for files that differ with
+the Auto-Merge (case 2) is considered unsafe, see [security
+pitfalls](#securityMergeCommits) below.
+
+## <a id="securityPitfalls">Security pitfalls
+
+While requiring code owner approvals is primarily considered as a code quality
+feature and not a security feature, many admins / projects owners are concerned
+about possibilities to bypass code owner approvals. These admins / projects
+owners should be aware that some configuration settings may make it possible to
+bypass code owner approvals, and hence using them is not recommended.
+
+### <a id="securityImplicitApprovals">Implicit approvals
+
+If [implicit approvals](#implicitApprovals) are enabled, it is important that
+code owners are aware of their implicit approval when they upload new patch sets
+for other users. E.g. if a contributor pushes a change to a wrong branch and a
+code owner helps them to get it rebased onto the correct branch, the rebased
+change has implicit approvals from the code owner, since the code owner is the
+uploader. To avoid situations like this it is recommended to not enable implicit
+approvals.
+
+### <a id="securityMergeCommits">Required code owner approvals on merge commits
+
+If any branch doesn't require code owner approvals or if the code owners in any
+branch are not trusted, it is not safe to [configure for merge commits that they
+only require code owner approvals for files that differ with the
+Auto-Merge](#mergeCommits). E.g. if there is a branch that doesn't require code
+owner approvals, with this setting the code owners check can be bypassed by:
+
+1. setting the branch that doesn't require code owner approvals to the same
+ commit as the main branch that does require code owner approvals
+2. making a change in the branch that doesn't require code owner approvals
+3. merging this change back into the main branch that does require code owner
+ approvals
+4. since it's a clean merge, all files are merged automatically and no code
+ owner approval is required
+
+### <a id="securityFallbackCodeOwners">Setting all users as fallback code owners
+
+As soon as the code owners functionality is enabled for a project / branch, all
+files in it require code owner approvals. This means if any path doesn't have
+any code owners defined, submitting changes to the path is only possible with
+
+1. a code owner override
+2. an approval from a fallback code owners (only if enabled)
+
+[Configuring all users as fallback code
+owners](config.html#pluginCodeOwnersFallbackCodeOwners) is problematic, as it
+can happen easily that code owner config files are misconfigured so that some
+paths are accidentally not covered by code owners. In this case, the affected
+paths would suddenly be open to all users, which may not be wanted. This is why
+configuring all users as fallback code owners is not recommended.
+
+---
+
+Back to [@PLUGIN@ documentation index](index.html)
+
+Part of [Gerrit Code Review](../../../Documentation/index.html)
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 24ed4b9..b01532a 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -3,6 +3,9 @@
The global configuration of the @PLUGIN@ plugin is stored in the `gerrit.config`
file in the `plugin.@PLUGIN@` subsection.
+This page describes all available configuration parameters. For configuration
+recommendations please consult the [config guide](#config-guide.html).
+
## <a id="projectLevelConfigFile">
In addition some configuration can be done on the project level in
`@PLUGIN@.config` files that are stored in the `refs/meta/config` branches of
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index da00ae4..bd25b37 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -188,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
diff --git a/resources/Documentation/setup-guide.md b/resources/Documentation/setup-guide.md
index c662be7..7449164 100644
--- a/resources/Documentation/setup-guide.md
+++ b/resources/Documentation/setup-guide.md
@@ -27,6 +27,9 @@
* [How to update the code-owners.config file for a project](#updateCodeOwnersConfig)
* [How to check if the code owners functionality is enabled for a project or branch](#checkIfEnabled)
+Recommendations about further configuration parameters can be found in the
+[config guide](config-guide.html).
+
### <a id="configureCodeOwnersBackend">1. Configure the code owners backend that should be used
The `code-owners` plugin supports multiple [code owner backends](backends.html)
