CheckCodeOwnerConfigFiles: Allow to specify branches that should be validated
This way you can limit the validation to those branches that you are
interested in.
The request is rejected if:
* any of the specified branches doesn't exist
* the code owners functionality is disabled for any of the specified
branches and validateDisabledBranches is false (or unset)
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I81f03cd940d03a363149d4916ffec488947f4684
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CheckCodeOwnerConfigFilesInput.java b/java/com/google/gerrit/plugins/codeowners/api/CheckCodeOwnerConfigFilesInput.java
index f06edf4..6e2abfc 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CheckCodeOwnerConfigFilesInput.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CheckCodeOwnerConfigFilesInput.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.codeowners.api;
+import java.util.List;
+
/**
* The input for the {@link com.google.gerrit.plugins.codeowners.restapi.CheckCodeOwnerConfigFiles}
* REST endpoint.
@@ -26,4 +28,14 @@
* <p>By default unset, {@code false}.
*/
public Boolean validateDisabledBranches;
+
+ /**
+ * Branches for which code owner config files should be validated.
+ *
+ * <p>The {@code refs/heads/} prefix may be omitted.
+ *
+ * <p>By default unset, which means that code owner config files in all branches should be
+ * validated.
+ */
+ public List<String> branches;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwners.java
index b2823ea..6f86d06 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwners.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.codeowners.api;
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import java.util.List;
@@ -34,6 +36,7 @@
/** Request to check code owner config files. */
abstract class CheckCodeOwnerConfigFilesRequest {
private boolean validateDisabledBranches;
+ private ImmutableList<String> branches;
/**
* Includes code owner config files in branches for which the code owners functionality is
@@ -61,6 +64,22 @@
return validateDisabledBranches;
}
+ /** Sets the branches for which code owner config files should be validated. */
+ public CheckCodeOwnerConfigFilesRequest setBranches(List<String> branches) {
+ this.branches = ImmutableList.copyOf(branches);
+ return this;
+ }
+
+ /**
+ * Gets the branches for which code owner config files should be validated.
+ *
+ * <p>Returns {@code null} if no branches have been set.
+ */
+ @Nullable
+ public ImmutableList<String> getBranches() {
+ return branches;
+ }
+
/**
* Executes the request to check the code owner config files and retrieves the result of the
* validation.
diff --git a/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwnersImpl.java b/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwnersImpl.java
index 750d98d..0a4fdad 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwnersImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/ProjectCodeOwnersImpl.java
@@ -84,6 +84,7 @@
try {
CheckCodeOwnerConfigFilesInput input = new CheckCodeOwnerConfigFilesInput();
input.validateDisabledBranches = isValidateDisabledBranches();
+ input.branches = getBranches();
return checkCodeOwnerConfigFiles.apply(projectResource, input).value();
} catch (Exception e) {
throw asRestApiException("Cannot check code owner config files", e);
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
index cb55949..a00f476 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
@@ -15,19 +15,24 @@
package com.google.gerrit.plugins.codeowners.restapi;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Multimaps;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.plugins.codeowners.api.CheckCodeOwnerConfigFilesInput;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
@@ -48,7 +53,6 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
-import java.util.stream.Stream;
/**
* REST endpoint that checks/validates the code owner config files in a project.
@@ -103,12 +107,18 @@
.check(ProjectPermission.WRITE_CONFIG);
logger.atFine().log(
- "checking code owner config files for project %s (validateDisabledBranches = %s)",
- projectResource.getNameKey(), input.validateDisabledBranches);
+ "checking code owner config files for project %s"
+ + " (validateDisabledBranches = %s, branches = %s)",
+ projectResource.getNameKey(), input.validateDisabledBranches, input.branches);
+
+ ImmutableSet<BranchNameKey> branches = branches(projectResource);
+
+ validateInput(projectResource.getNameKey(), branches, input);
ImmutableMap.Builder<String, Map<String, List<ConsistencyProblemInfo>>> resultsByBranchBuilder =
ImmutableMap.builder();
- branches(projectResource)
+ branches.stream()
+ .filter(branchNameKey -> shouldValidateBranch(input, branchNameKey))
.filter(
branchNameKey ->
validateDisabledBranches(input)
@@ -119,11 +129,12 @@
return Response.ok(resultsByBranchBuilder.build());
}
- private Stream<BranchNameKey> branches(ProjectResource projectResource)
+ private ImmutableSet<BranchNameKey> branches(ProjectResource projectResource)
throws RestApiException, IOException, PermissionBackendException {
return listBranches.get().apply(projectResource).value().stream()
.filter(branchInfo -> !"HEAD".equals(branchInfo.ref))
- .map(branchInfo -> BranchNameKey.create(projectResource.getNameKey(), branchInfo.ref));
+ .map(branchInfo -> BranchNameKey.create(projectResource.getNameKey(), branchInfo.ref))
+ .collect(toImmutableSet());
}
private Map<String, List<ConsistencyProblemInfo>> checkBranch(BranchNameKey branchNameKey) {
@@ -179,6 +190,43 @@
commitValidationMessage.getType(), commitValidationMessage.getMessage()));
}
+ private void validateInput(
+ Project.NameKey projectName,
+ ImmutableSet<BranchNameKey> branches,
+ CheckCodeOwnerConfigFilesInput input)
+ throws RestApiException {
+ if (input.branches != null) {
+ for (String branchName : input.branches) {
+ BranchNameKey branchNameKey = BranchNameKey.create(projectName, branchName);
+ if (!branches.contains(branchNameKey)) {
+ throw new UnprocessableEntityException(String.format("branch %s not found", branchName));
+ }
+
+ if ((input.validateDisabledBranches == null || !input.validateDisabledBranches)
+ && codeOwnersPluginConfiguration.isDisabled(branchNameKey)) {
+ throw new BadRequestException(
+ String.format(
+ "code owners functionality for branch %s is disabled,"
+ + " set 'validate_disabled_braches' in the input to 'true' if code owner"
+ + " config files in this branch should be validated",
+ branchName));
+ }
+ }
+ }
+ }
+
+ private static boolean shouldValidateBranch(
+ CheckCodeOwnerConfigFilesInput input, BranchNameKey branchNameKey) {
+ boolean shouldValidateBranch =
+ input.branches == null
+ || input.branches.contains(branchNameKey.branch())
+ || input.branches.contains(branchNameKey.shortName());
+ if (!shouldValidateBranch) {
+ logger.atFine().log("skip validation for branch %s", branchNameKey.branch());
+ }
+ return shouldValidateBranch;
+ }
+
private static boolean validateDisabledBranches(CheckCodeOwnerConfigFilesInput input) {
return input.validateDisabledBranches != null && input.validateDisabledBranches;
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
index f742ffe..a2f5572 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
@@ -16,16 +16,23 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
+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.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
+import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.plugins.codeowners.JgitPath;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
@@ -53,6 +60,7 @@
*/
public class CheckCodeOwnerConfigFilesIT extends AbstractCodeOwnersIT {
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ProjectOperations projectOperations;
private BackendConfig backendConfig;
@@ -209,6 +217,106 @@
"refs/meta/config", ImmutableMap.of());
}
+ @Test
+ public void validateSpecifiedBranches() throws Exception {
+ gApi.projects().name(project.get()).branch("stable-1.0").create(new BranchInput());
+ gApi.projects().name(project.get()).branch("stable-1.1").create(new BranchInput());
+
+ assertThat(
+ projectCodeOwnersApiFactory
+ .project(project)
+ .checkCodeOwnerConfigFiles()
+ .setBranches(ImmutableList.of("refs/heads/stable-1.0", "refs/heads/stable-1.1"))
+ .check())
+ .containsExactly(
+ "refs/heads/stable-1.0", ImmutableMap.of(),
+ "refs/heads/stable-1.1", ImmutableMap.of());
+ }
+
+ @Test
+ public void validateSpecifiedBranches_shortNames() throws Exception {
+ gApi.projects().name(project.get()).branch("stable-1.0").create(new BranchInput());
+ gApi.projects().name(project.get()).branch("stable-1.1").create(new BranchInput());
+
+ assertThat(
+ projectCodeOwnersApiFactory
+ .project(project)
+ .checkCodeOwnerConfigFiles()
+ .setBranches(ImmutableList.of("stable-1.0", "stable-1.1"))
+ .check())
+ .containsExactly(
+ "refs/heads/stable-1.0", ImmutableMap.of(),
+ "refs/heads/stable-1.1", ImmutableMap.of());
+ }
+
+ @Test
+ public void cannotValidateNonExistingBranch() throws Exception {
+ UnprocessableEntityException exception =
+ assertThrows(
+ UnprocessableEntityException.class,
+ () ->
+ projectCodeOwnersApiFactory
+ .project(project)
+ .checkCodeOwnerConfigFiles()
+ .setBranches(ImmutableList.of("refs/heads/non-existing"))
+ .check());
+ assertThat(exception).hasMessageThat().isEqualTo("branch refs/heads/non-existing not found");
+ }
+
+ @Test
+ public void cannotValidateNonVisibleBranch() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+
+ UnprocessableEntityException exception =
+ assertThrows(
+ UnprocessableEntityException.class,
+ () ->
+ projectCodeOwnersApiFactory
+ .project(project)
+ .checkCodeOwnerConfigFiles()
+ .setBranches(ImmutableList.of("refs/heads/master"))
+ .check());
+ assertThat(exception).hasMessageThat().isEqualTo("branch refs/heads/master not found");
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.disabledBranch", value = "refs/meta/config")
+ public void cannotValidateDisabledBranchWithoutEnablingValidationForDisabledBranches()
+ throws Exception {
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ projectCodeOwnersApiFactory
+ .project(project)
+ .checkCodeOwnerConfigFiles()
+ .setBranches(ImmutableList.of("refs/meta/config"))
+ .check());
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ "code owners functionality for branch refs/meta/config is disabled,"
+ + " set 'validate_disabled_braches' in the input to 'true' if code owner config"
+ + " files in this branch should be validated");
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.disabledBranch", value = "refs/meta/config")
+ public void validateSpecifiedBranchThatIsDisabled() throws Exception {
+ assertThat(
+ projectCodeOwnersApiFactory
+ .project(project)
+ .checkCodeOwnerConfigFiles()
+ .validateDisabledBranches()
+ .setBranches(ImmutableList.of("refs/meta/config"))
+ .check())
+ .containsExactly("refs/meta/config", ImmutableMap.of());
+ }
+
private ConsistencyProblemInfo error(String message) {
return new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, message);
}
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 5c23d5b..189ff23 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -382,6 +382,7 @@
| Field Name | | Description |
| ---------------------------- | -------- | ----------- |
| `validate_disabled_branches` | optional | Whether code owner config files in branches for which the code owners functionality is disabled should be validated too. By default unset, `false`.
+| `branches` | optional | List of branches for which code owner config files should be validated. The `refs/heads/` prefix may be omitted. By default unset, which means that code owner config files in all branches should be validated.
---
