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. ---