Merge branch 'stable-3.5'
* stable-3.5:
Remove the bug-report link from code-owners
Use PerThreadProjectCache instead of PerThreadCache
Change-Id: Ib37ccacffe8db1ff31ac30907dd437ffbd79665f
diff --git a/README.md b/README.md
index edb073e..3767276 100644
--- a/README.md
+++ b/README.md
@@ -5,10 +5,18 @@
If the code-owners plugin is enabled, changes can only be submitted if all
touched files are covered by approvals from code owners.
-Also see [resources/Documentation/about.md](./resources/Documentation/about.md).
+For a detailed description of the plugin functionality please refer to the
+[plugin documentation](https://android-review.googlesource.com/plugins/code-owners/Documentation/index.html).
IMPORTANT: Before installing/enabling the plugin follow the instructions from
-the setup guide, see [resources/Documentation/setup-guide.md](./resources/Documentation/setup-guide.md).
+the [setup guide](https://android-review.googlesource.com/plugins/code-owners/Documentation/setup-guide.html).
+
+NOTE: The plugin documentation only renders correctly when the plugin is
+installed in Gerrit and the documentation is accessed via
+https://<gerrit-host>/plugins/code-owners/Documentation/index.html. If you want
+to read the documentation before installing the plugin, you can find it properly
+rendered
+[here](https://android-review.googlesource.com/plugins/code-owners/Documentation/index.html).
## JavaScript Plugin
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
index 1309568..32c82a1 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
@@ -191,7 +191,7 @@
.message("Configure code owner backend")
.add("code-owners.config", codeOwnersConfig.toText()));
}
- projectCache.evict(project);
+ projectCache.evictAndReindex(project);
}
protected void createOwnersOverrideLabel() throws RestApiException {
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/TestBatchModule.java b/java/com/google/gerrit/plugins/codeowners/acceptance/TestBatchModule.java
new file mode 100644
index 0000000..c6170c5
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/TestBatchModule.java
@@ -0,0 +1,35 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.acceptance;
+
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperationsImpl;
+import com.google.gerrit.plugins.codeowners.module.BatchModule;
+import com.google.inject.AbstractModule;
+
+/**
+ * Guice module that makes the code-owners batch module and the code owners test API available
+ * during the test execution
+ */
+public class TestBatchModule extends AbstractModule {
+ @Override
+ public void configure() {
+ install(new BatchModule());
+
+ // Only add bindings here that are specifically required for tests, in order to keep the Guice
+ // setup in tests as realistic as possible by delegating to the original module.
+ bind(CodeOwnerConfigOperations.class).to(CodeOwnerConfigOperationsImpl.class);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java
index 8bc182e..bd9d0f5 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java
@@ -98,6 +98,9 @@
/** Whether pure revert changes are exempted from needing code owner approvals for submit. */
public Boolean exemptPureReverts;
+ /** Policy for validating code owner config files when a branch is created. */
+ public CodeOwnerConfigValidationPolicy enableValidationOnBranchCreation;
+
/** Policy for validating code owner config files when a commit is received. */
public CodeOwnerConfigValidationPolicy enableValidationOnCommitReceived;
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java b/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
index 2bf9bff..a0df99d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
@@ -113,10 +114,10 @@
fileName, codeOwnerConfigParser, revWalk, revision, codeOwnerConfigKey);
}
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("failed to load code owner config %s", codeOwnerConfigKey), e);
} catch (ConfigInvalidException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"invalid code owner config file %s (project = %s, branch = %s)",
codeOwnerConfigKey.filePath(defaultFileName),
@@ -209,7 +210,7 @@
.call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("failed to upsert code owner config %s", codeOwnerConfigKey), e);
}
}
@@ -257,7 +258,7 @@
return codeOwnerConfigFile.getLoadedCodeOwnerConfig();
} catch (IOException | ConfigInvalidException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("failed to upsert code owner config %s", codeOwnerConfigKey), e);
}
}
@@ -279,7 +280,7 @@
}
return metaDataUpdate;
} catch (Exception e) {
- throw new CodeOwnersInternalServerErrorException("Failed to create MetaDataUpdate", e);
+ throw newInternalServerError("Failed to create MetaDataUpdate", e);
} finally {
metaDataUpdate.close();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index 81f31b3..27412f2 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -31,6 +31,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
+import com.google.gerrit.server.patch.DiffOptions;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -97,7 +98,8 @@
// For merge commits the default base is the auto-merge commit which should be used as base
// if the merge commit strategy is FILES_WITH_CONFLICT_RESOLUTION.
fileDiffOutputs =
- diffOperations.listModifiedFilesAgainstParent(project, revision, /* parentNum=*/ 0);
+ diffOperations.listModifiedFilesAgainstParent(
+ project, revision, /* parentNum=*/ 0, DiffOptions.DEFAULTS);
} else {
checkState(mergeCommitStrategy.equals(MergeCommitStrategy.ALL_CHANGED_FILES));
// Always use parent 1 to do the comparison.
@@ -105,7 +107,9 @@
// handled above).
// For merge commits also the first parent should be used if the merge commit strategy is
// ALL_CHANGED_FILES.
- fileDiffOutputs = diffOperations.listModifiedFilesAgainstParent(project, revision, 1);
+ fileDiffOutputs =
+ diffOperations.listModifiedFilesAgainstParent(
+ project, revision, 1, DiffOptions.DEFAULTS);
}
return toChangedFiles(filterOutMagicFilesAndSort(fileDiffOutputs)).collect(toImmutableList());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index f015cb0..a04c1ad 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
@@ -30,10 +31,8 @@
import com.google.gerrit.entities.LabelValue;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
-import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -47,9 +46,6 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.DiffNotAvailableException;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.util.AccountTemplateUtil;
import com.google.gerrit.server.util.LabelVote;
import com.google.inject.Inject;
@@ -86,7 +82,6 @@
public class CodeOwnerApprovalCheck {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private final PermissionBackend permissionBackend;
private final GitRepositoryManager repoManager;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final ChangedFiles changedFiles;
@@ -98,7 +93,6 @@
@Inject
CodeOwnerApprovalCheck(
- PermissionBackend permissionBackend,
GitRepositoryManager repoManager,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
ChangedFiles changedFiles,
@@ -107,7 +101,6 @@
Provider<CodeOwnerResolver> codeOwnerResolverProvider,
ApprovalsUtil approvalsUtil,
CodeOwnerMetrics codeOwnerMetrics) {
- this.permissionBackend = permissionBackend;
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.changedFiles = changedFiles;
@@ -127,11 +120,9 @@
* @param start number of owned paths to skip
* @param limit the max number of owned paths that should be returned (0 = unlimited)
* @return the paths of the files in the given patch set that are owned by the specified account
- * @throws ResourceConflictException if the destination branch of the change no longer exists
*/
public ImmutableList<OwnedChangedFile> getOwnedPaths(
- ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit)
- throws ResourceConflictException {
+ ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit) {
try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
logger.atFine().log(
"compute owned paths for account %d (project = %s, change = %d, patch set = %d,"
@@ -182,7 +173,7 @@
.orElse(null)))
.collect(toImmutableList());
} catch (IOException | DiffNotAvailableException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"failed to compute owned paths of patch set %s for account %d",
patchSet.id(), accountId.get()),
@@ -197,7 +188,7 @@
* @return whether the given change has sufficient code owner approvals to be submittable
*/
public boolean isSubmittable(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
logger.atFine().log(
"checking if change %d in project %s is submittable",
@@ -242,8 +233,7 @@
* @see #getFileStatuses(CodeOwnerConfigHierarchy, CodeOwnerResolver, ChangeNotes)
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
- ChangeNotes changeNotes, int start, int limit)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ ChangeNotes changeNotes, int start, int limit) throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
logger.atFine().log(
@@ -296,7 +286,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputation.start()) {
logger.atFine().log(
@@ -365,8 +355,13 @@
overrides);
BranchNameKey branch = changeNotes.getChange().getDest();
- ObjectId revision = getDestBranchRevision(changeNotes.getChange());
- logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
+ Optional<ObjectId> revision = getDestBranchRevision(changeNotes.getChange());
+ if (revision.isPresent()) {
+ logger.atFine().log(
+ "dest branch %s has revision %s", branch.branch(), revision.get().name());
+ } else {
+ logger.atFine().log("dest branch %s does not exist", branch.branch());
+ }
CodeOwnerResolverResult globalCodeOwners =
codeOwnerResolver.resolveGlobalCodeOwners(changeNotes.getProjectName());
@@ -390,7 +385,7 @@
codeOwnerConfigHierarchy,
codeOwnerResolver,
branch,
- revision,
+ revision.orElse(null),
globalCodeOwners,
enableImplicitApproval ? changeOwner : null,
reviewerAccountIds,
@@ -417,7 +412,7 @@
@VisibleForTesting
public Stream<FileCodeOwnerStatus> getFileStatusesForAccount(
ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
requireNonNull(patchSet, "patchSet");
requireNonNull(accountId, "accountId");
@@ -437,13 +432,16 @@
logger.atFine().log("requiredApproval = %s", requiredApproval);
BranchNameKey branch = changeNotes.getChange().getDest();
- ObjectId revision = getDestBranchRevision(changeNotes.getChange());
- logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
+ Optional<ObjectId> revision = getDestBranchRevision(changeNotes.getChange());
+ if (revision.isPresent()) {
+ logger.atFine().log(
+ "dest branch %s has revision %s", branch.branch(), revision.get().name());
+ } else {
+ logger.atFine().log("dest branch %s does not exist", branch.branch());
+ }
- boolean isProjectOwner = isProjectOwner(changeNotes.getProjectName(), accountId);
FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
- logger.atFine().log(
- "fallbackCodeOwner = %s, isProjectOwner = %s", fallbackCodeOwners, isProjectOwner);
+ logger.atFine().log("fallbackCodeOwner = %s", fallbackCodeOwners);
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
CodeOwnerResolver codeOwnerResolver =
@@ -456,7 +454,7 @@
codeOwnerConfigHierarchy,
codeOwnerResolver,
branch,
- revision,
+ revision.orElse(null),
/* globalCodeOwners= */ CodeOwnerResolverResult.createEmpty(),
// Do not check for implicit approvals since implicit approvals of other users
// should be ignored. For the given account we do not need to check for
@@ -477,7 +475,7 @@
return changeNotes.getChange().getRevertOf() != null
&& pureRevertCache.isPureRevert(changeNotes);
} catch (BadRequestException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"failed to check if change %s in project %s is a pure revert",
changeNotes.getChangeId(), changeNotes.getProjectName()),
@@ -512,7 +510,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
- ObjectId revision,
+ @Nullable ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -577,7 +575,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
- ObjectId revision,
+ @Nullable ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -680,7 +678,6 @@
CodeOwnerStatus codeOwnerStatusForFallbackCodeOwners =
getCodeOwnerStatusForFallbackCodeOwners(
codeOwnerStatus.get(),
- branch,
implicitApprover,
reviewerAccountIds,
approverAccountIds,
@@ -722,108 +719,10 @@
}
/**
- * Gets the code owner status for the given path when project owners are configured as fallback
- * code owners.
- */
- private CodeOwnerStatus getCodeOwnerStatusForProjectOwnersAsFallbackCodeOwners(
- BranchNameKey branch,
- @Nullable Account.Id implicitApprover,
- ImmutableSet<Account.Id> reviewerAccountIds,
- ImmutableSet<Account.Id> approverAccountIds,
- Path absolutePath,
- AtomicReference<String> reason) {
- logger.atFine().log(
- "computing code owner status for %s with project owners as fallback code owners",
- absolutePath);
-
- CodeOwnerStatus codeOwnerStatus = CodeOwnerStatus.INSUFFICIENT_REVIEWERS;
- if (isApprovedByProjectOwner(branch.project(), approverAccountIds, implicitApprover, reason)) {
- codeOwnerStatus = CodeOwnerStatus.APPROVED;
- } else if (isPendingByProjectOwner(branch.project(), reviewerAccountIds, reason)) {
- codeOwnerStatus = CodeOwnerStatus.PENDING;
- }
-
- return codeOwnerStatus;
- }
-
- private boolean isApprovedByProjectOwner(
- Project.NameKey projectName,
- ImmutableSet<Account.Id> approverAccountIds,
- @Nullable Account.Id implicitApprover,
- AtomicReference<String> reason) {
- return (implicitApprover != null
- && isImplicitlyApprovedByProjectOwner(projectName, implicitApprover, reason))
- || isExplicitlyApprovedByProjectOwner(projectName, approverAccountIds, reason);
- }
-
- private boolean isImplicitlyApprovedByProjectOwner(
- Project.NameKey projectName, Account.Id implicitApprover, AtomicReference<String> reason) {
- requireNonNull(implicitApprover, "implicitApprover");
- if (isProjectOwner(projectName, implicitApprover)) {
- // The uploader of the patch set is a project owner and thus a code owner. This means there
- // is an implicit code owner approval from the patch set uploader so that the path is
- // automatically approved.
- reason.set(
- String.format(
- "implicitly approved by the patch set uploader %s who is a %s"
- + " (all project owners are %ss)",
- AccountTemplateUtil.getAccountTemplate(implicitApprover),
- CodeOwnerKind.FALLBACK_CODE_OWNER.getDisplayName(),
- CodeOwnerKind.FALLBACK_CODE_OWNER.getDisplayName()));
- return true;
- }
- return false;
- }
-
- private boolean isExplicitlyApprovedByProjectOwner(
- Project.NameKey projectName,
- ImmutableSet<Account.Id> approverAccountIds,
- AtomicReference<String> reason) {
- Optional<Account.Id> approver =
- approverAccountIds.stream()
- .filter(approverAccountId -> isProjectOwner(projectName, approverAccountId))
- .findAny();
- if (approver.isPresent()) {
- // At least one of the approvers is a project owner and thus a code owner.
- reason.set(
- String.format(
- "approved by %s who is a %s (all project owners are %ss)",
- AccountTemplateUtil.getAccountTemplate(approver.get()),
- CodeOwnerKind.FALLBACK_CODE_OWNER.getDisplayName(),
- CodeOwnerKind.FALLBACK_CODE_OWNER.getDisplayName()));
- return true;
- }
- return false;
- }
-
- private boolean isPendingByProjectOwner(
- Project.NameKey projectName,
- ImmutableSet<Account.Id> reviewerAccountIds,
- AtomicReference<String> reason) {
- Optional<Account.Id> reviewer =
- reviewerAccountIds.stream()
- .filter(reviewerAccountId -> isProjectOwner(projectName, reviewerAccountId))
- .findAny();
- if (reviewer.isPresent()) {
- // At least one of the reviewers is a project owner and thus a code owner.
- reason.set(
- String.format(
- "reviewer %s is a %s (all project owners are %ss)",
- AccountTemplateUtil.getAccountTemplate(reviewer.get()),
- CodeOwnerKind.FALLBACK_CODE_OWNER.getDisplayName(),
- CodeOwnerKind.FALLBACK_CODE_OWNER.getDisplayName()));
- return true;
- }
-
- return false;
- }
-
- /**
* Computes the code owner status for the given path based on the configured fallback code owners.
*/
private CodeOwnerStatus getCodeOwnerStatusForFallbackCodeOwners(
CodeOwnerStatus codeOwnerStatus,
- BranchNameKey branch,
@Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
@@ -837,15 +736,12 @@
case NONE:
logger.atFine().log("no fallback code owners");
return codeOwnerStatus;
- case PROJECT_OWNERS:
- return getCodeOwnerStatusForProjectOwnersAsFallbackCodeOwners(
- branch, implicitApprover, reviewerAccountIds, approverAccountIds, absolutePath, reason);
case ALL_USERS:
return getCodeOwnerStatusIfAllUsersAreCodeOwners(
implicitApprover, reviewerAccountIds, approverAccountIds, absolutePath, reason);
}
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("unknown fallback code owners configured: %s", fallbackCodeOwners));
}
@@ -992,27 +888,6 @@
return false;
}
- /** Whether the given account is a project owner of the given project. */
- private boolean isProjectOwner(Project.NameKey project, Account.Id accountId) {
- try {
- boolean isProjectOwner =
- permissionBackend
- .absentUser(accountId)
- .project(project)
- .test(ProjectPermission.WRITE_CONFIG);
- if (isProjectOwner) {
- logger.atFine().log("Account %d is a project owner", accountId.get());
- }
- return isProjectOwner;
- } catch (PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException(
- String.format(
- "failed to check owner permission of project %s for account %d",
- project.get(), accountId.get()),
- e);
- }
- }
-
/**
* Resolves the given path code owners.
*
@@ -1077,13 +952,7 @@
private ImmutableList<PatchSetApproval> getCurrentPatchSetApprovals(ChangeNotes changeNotes) {
try (Timer0.Context ctx = codeOwnerMetrics.computePatchSetApprovals.start()) {
return ImmutableList.copyOf(
- approvalsUtil.byPatchSet(
- changeNotes,
- changeNotes.getCurrentPatchSet().id(),
- /** revWalk */
- null,
- /** repoConfig */
- null));
+ approvalsUtil.byPatchSet(changeNotes, changeNotes.getCurrentPatchSet().id()));
}
}
@@ -1142,18 +1011,18 @@
* <p>This is the revision from which the code owner configs should be read when computing code
* owners for the files that are touched in the change.
*
- * @throws ResourceConflictException thrown if the destination branch is not found, e.g. when the
- * branch got deleted after the change was created
+ * @return the current revision of the destination branch of the given change, {@link
+ * Optional#empty()} if the destination branch is not found (e.g. when the initial change is
+ * uploaded to an unborn branch or when the branch got deleted after the change was created)
*/
- private ObjectId getDestBranchRevision(Change change)
- throws IOException, ResourceConflictException {
+ private Optional<ObjectId> getDestBranchRevision(Change change) throws IOException {
try (Repository repository = repoManager.openRepository(change.getProject());
RevWalk rw = new RevWalk(repository)) {
Ref ref = repository.exactRef(change.getDest().branch());
if (ref == null) {
- throw new ResourceConflictException("destination branch not found");
+ return Optional.empty();
}
- return rw.parseCommit(ref.getObjectId());
+ return Optional.of(rw.parseCommit(ref.getObjectId()));
}
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
index 9e15746..d0bd003 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
@@ -140,7 +141,7 @@
updateBranch(branchNameKey.branch(), repository, revision, commitId);
return Optional.of(rw.parseCommit(commitId));
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"Failed to scan for code owner configs in branch %s of project %s",
branchNameKey.branch(), branchNameKey.project()),
@@ -183,8 +184,7 @@
cb.setParentId(parentCommit);
cb.setTreeId(treeId);
cb.setCommitter(serverIdent);
- cb.setAuthor(
- identifiedUser.get().newCommitterIdent(serverIdent.getWhen(), serverIdent.getTimeZone()));
+ cb.setAuthor(identifiedUser.get().newCommitterIdent(serverIdent));
cb.setMessage(commitMessage);
ObjectId id = objectInserter.insert(cb);
objectInserter.flush();
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index 7ba3352..28dd5e1 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -15,9 +15,11 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
@@ -42,6 +44,10 @@
* config in the root folder of the branch. The same as any other parent it can be ignored (e.g. by
* using {@code set noparent} in the root code owner config if the {@code find-owners} backend is
* used).
+ *
+ * <p>Visiting the code owner configs also works for non-existing branches (provided branch revision
+ * is {@code null}). In this case only the default code owner config in {@code refs/meta/config} is
+ * visited (if it exists).
*/
public class CodeOwnerConfigHierarchy {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -65,7 +71,8 @@
* the path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner
@@ -73,7 +80,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
CodeOwnerConfigVisitor codeOwnerConfigVisitor) {
visit(
@@ -89,7 +96,8 @@
* the path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner
@@ -99,7 +107,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
CodeOwnerConfigVisitor codeOwnerConfigVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -119,7 +127,8 @@
* path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param pathCodeOwnersVisitor visitor that should be invoked for the applying path code owners
@@ -128,7 +137,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -153,7 +162,8 @@
* (e.g. for large changes).
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absoluteFilePath the path for which the code owner configs should be visited; the path
* must be absolute; must be the path of a file; the path may or may not exist
* @param pathCodeOwnersVisitor visitor that should be invoked for the applying path code owners
@@ -162,7 +172,7 @@
*/
public void visitForFile(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absoluteFilePath,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -177,13 +187,12 @@
private void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
Path startFolder,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
requireNonNull(branchNameKey, "branch");
- requireNonNull(revision, "revision");
requireNonNull(absolutePath, "absolutePath");
requireNonNull(pathCodeOwnersVisitor, "pathCodeOwnersVisitor");
requireNonNull(parentCodeOwnersIgnoredCallback, "parentCodeOwnersIgnoredCallback");
@@ -191,47 +200,52 @@
logger.atFine().log(
"visiting code owner configs for '%s' in branch '%s' in project '%s' (revision = '%s')",
- absolutePath, branchNameKey.shortName(), branchNameKey.project(), revision.name());
+ absolutePath,
+ branchNameKey.shortName(),
+ branchNameKey.project(),
+ revision != null ? revision.name() : "n/a");
- // Next path in which we look for a code owner configuration. We start at the given folder and
- // then go up the parent hierarchy.
- Path ownerConfigFolder = startFolder;
+ if (revision != null) {
+ // Next path in which we look for a code owner configuration. We start at the given folder and
+ // then go up the parent hierarchy.
+ Path ownerConfigFolder = startFolder;
- // Iterate over the parent code owner configurations.
- while (ownerConfigFolder != null) {
- // Read code owner config and invoke the codeOwnerConfigVisitor if the code owner config
- // exists.
- logger.atFine().log("inspecting code owner config for %s", ownerConfigFolder);
- CodeOwnerConfig.Key codeOwnerConfigKey =
- CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
- Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(
- transientCodeOwnerConfigCache, codeOwnerConfigKey, revision, absolutePath);
- if (pathCodeOwners.isPresent()) {
- logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
- boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
- boolean ignoreParentCodeOwners =
- pathCodeOwners.get().resolveCodeOwnerConfig().get().ignoreParentCodeOwners();
- if (ignoreParentCodeOwners) {
- parentCodeOwnersIgnoredCallback.accept(codeOwnerConfigKey);
+ // Iterate over the parent code owner configurations.
+ while (ownerConfigFolder != null) {
+ // Read code owner config and invoke the codeOwnerConfigVisitor if the code owner config
+ // exists.
+ logger.atFine().log("inspecting code owner config for %s", ownerConfigFolder);
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
+ Optional<PathCodeOwners> pathCodeOwners =
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCache, codeOwnerConfigKey, revision, absolutePath);
+ if (pathCodeOwners.isPresent()) {
+ logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
+ boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
+ boolean ignoreParentCodeOwners =
+ pathCodeOwners.get().resolveCodeOwnerConfig().get().ignoreParentCodeOwners();
+ if (ignoreParentCodeOwners) {
+ parentCodeOwnersIgnoredCallback.accept(codeOwnerConfigKey);
+ }
+ logger.atFine().log(
+ "visitFurtherCodeOwnerConfigs = %s, ignoreParentCodeOwners = %s",
+ visitFurtherCodeOwnerConfigs, ignoreParentCodeOwners);
+ if (!visitFurtherCodeOwnerConfigs || ignoreParentCodeOwners) {
+ // If no further code owner configs should be visited or if all parent code owner
+ // configs are ignored, we are done.
+ // No need to check further parent code owner configs (including the default code owner
+ // config in refs/meta/config which is the parent of the root code owner config), hence
+ // we can return here.
+ return;
+ }
+ } else {
+ logger.atFine().log("no code owner config found in %s", ownerConfigFolder);
}
- logger.atFine().log(
- "visitFurtherCodeOwnerConfigs = %s, ignoreParentCodeOwners = %s",
- visitFurtherCodeOwnerConfigs, ignoreParentCodeOwners);
- if (!visitFurtherCodeOwnerConfigs || ignoreParentCodeOwners) {
- // If no further code owner configs should be visited or if all parent code owner configs
- // are ignored, we are done.
- // No need to check further parent code owner configs (including the default code owner
- // config in refs/meta/config which is the parent of the root code owner config), hence we
- // can return here.
- return;
- }
- } else {
- logger.atFine().log("no code owner config found in %s", ownerConfigFolder);
+
+ // Continue the loop with the next parent folder.
+ ownerConfigFolder = ownerConfigFolder.getParent();
}
-
- // Continue the loop with the next parent folder.
- ownerConfigFolder = ownerConfigFolder.getParent();
}
if (!RefNames.REFS_CONFIG.equals(branchNameKey.branch())) {
@@ -280,8 +294,7 @@
logger.atFine().log("code owner config %s not found", metaCodeOwnerConfigKey);
}
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
- String.format("failed to read %s", metaCodeOwnerConfigKey), e);
+ throw newInternalServerError(String.format("failed to read %s", metaCodeOwnerConfigKey), e);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReference.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReference.java
index 7f9d4c4..dff2506 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReference.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReference.java
@@ -72,7 +72,7 @@
/** The name of the code owner config file. */
public String fileName() {
- return filePath().getFileName().toString();
+ return Optional.ofNullable(filePath().getFileName()).map(Path::toString).orElse("");
}
/** User-readable string representing this code owner config reference. */
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
index 20d7a4c..d55ff8f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
@@ -15,6 +15,7 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidCodeOwnerConfigCause;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.flogger.FluentLogger;
@@ -162,7 +163,7 @@
}
}
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"Failed to scan for code owner configs in branch %s of project %s",
branchNameKey.branch(), branchNameKey.project()),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 254b3a6..07894ce 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
@@ -258,7 +259,7 @@
public CodeOwnerResolverResult resolve(Set<CodeOwnerReference> codeOwnerReferences) {
return resolve(
codeOwnerReferences,
- /* annotations= */ ImmutableMultimap.of(),
+ /* annotationsByCodeOwnerReference= */ ImmutableMultimap.of(),
/* unresolvedImports= */ ImmutableList.of(),
/* pathCodeOwnersMessages= */ ImmutableList.of());
}
@@ -595,7 +596,7 @@
});
return extIdsByEmail;
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("cannot resolve code owner emails: %s", emails), e);
}
}
@@ -868,7 +869,7 @@
return true;
}
} catch (PermissionBackendException ex) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"failed to test the %s global capability", GlobalPermission.MODIFY_ACCOUNT),
ex);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index 4053413..80300d6 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableList;
@@ -21,8 +22,6 @@
import com.google.gerrit.entities.LegacySubmitRequirement;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.annotations.Exports;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
@@ -42,6 +41,7 @@
@Singleton
class CodeOwnerSubmitRule implements SubmitRule {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ public static final String RULE_NAME = "Code-Owners";
public static class CodeOwnerSubmitRuleModule extends AbstractModule {
@Override
@@ -53,10 +53,7 @@
}
private static final LegacySubmitRequirement SUBMIT_REQUIREMENT =
- LegacySubmitRequirement.builder()
- .setFallbackText("Code Owners")
- .setType("code-owners")
- .build();
+ LegacySubmitRequirement.builder().setFallbackText(RULE_NAME).setType("code-owners").build();
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
@@ -97,12 +94,6 @@
return Optional.of(getSubmitRecord(changeData.notes()));
}
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- String.format(
- "Couldn't evaluate code owner statuses for patch set %d of change %d.",
- changeData.currentPatchSet().id().get(), changeData.change().getId().get()));
- return Optional.of(notReady());
} catch (Exception e) {
// Whether the exception should be treated as RULE_ERROR.
// RULE_ERROR must only be returned if the exception is caused by user misconfiguration (e.g.
@@ -157,15 +148,15 @@
if (isRuleError) {
codeOwnerMetrics.countCodeOwnerSubmitRuleErrors.increment(cause);
- logger.atWarning().log(errorMessage);
+ logger.atWarning().log("%s", errorMessage);
return Optional.of(ruleError(errorMessage));
}
- throw new CodeOwnersInternalServerErrorException(errorMessage, e);
+ throw newInternalServerError(errorMessage, e);
}
}
private SubmitRecord getSubmitRecord(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
return codeOwnerApprovalCheck.isSubmittable(changeNotes) ? ok() : notReady();
}
@@ -174,6 +165,7 @@
SubmitRecord submitRecord = new SubmitRecord();
submitRecord.status = SubmitRecord.Status.OK;
submitRecord.requirements = ImmutableList.of(SUBMIT_REQUIREMENT);
+ submitRecord.ruleName = RULE_NAME;
return submitRecord;
}
@@ -181,6 +173,7 @@
SubmitRecord submitRecord = new SubmitRecord();
submitRecord.status = SubmitRecord.Status.NOT_READY;
submitRecord.requirements = ImmutableList.of(SUBMIT_REQUIREMENT);
+ submitRecord.ruleName = RULE_NAME;
return submitRecord;
}
@@ -188,6 +181,7 @@
SubmitRecord submitRecord = new SubmitRecord();
submitRecord.errorMessage = reason;
submitRecord.status = SubmitRecord.Status.RULE_ERROR;
+ submitRecord.ruleName = RULE_NAME;
return submitRecord;
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
index c7da676..5eb316c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
@@ -52,9 +52,7 @@
@Override
public boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
- return isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)
- || isInvalidPathException(throwable);
+ return isCausedByConfigurationError(throwable);
}
@Override
@@ -102,9 +100,7 @@
@Override
public Optional<Status> getStatus(Throwable throwable) {
- if (isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)
- || isInvalidPathException(throwable)) {
+ if (isCausedByConfigurationError(throwable)) {
return Optional.of(Status.create(409, "Conflict"));
}
return Optional.empty();
@@ -115,6 +111,28 @@
return getCause(CodeOwnersInternalServerErrorException.class, throwable);
}
+ public static boolean isCausedByConfigurationError(Throwable throwable) {
+ return isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable)
+ || isInvalidPathException(throwable);
+ }
+
+ public static Optional<? extends Exception> getCauseOfConfigurationError(Throwable throwable) {
+ Optional<InvalidPathException> invalidPathException =
+ CodeOwnersExceptionHook.getInvalidPathException(throwable);
+ if (invalidPathException.isPresent()) {
+ return invalidPathException;
+ }
+
+ Optional<InvalidPluginConfigurationException> invalidPluginConfigurationException =
+ CodeOwnersExceptionHook.getInvalidPluginConfigurationCause(throwable);
+ if (invalidPluginConfigurationException.isPresent()) {
+ return invalidPluginConfigurationException;
+ }
+
+ return CodeOwners.getInvalidCodeOwnerConfigCause(throwable);
+ }
+
private static boolean isInvalidPluginConfigurationException(Throwable throwable) {
return getInvalidPluginConfigurationCause(throwable).isPresent();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorException.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorException.java
index 13b1a9d..580414a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorException.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorException.java
@@ -14,21 +14,65 @@
package com.google.gerrit.plugins.codeowners.backend;
+import com.google.common.base.Throwables;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
+
/** Exception signaling an internal server error in the code-owners plugin. */
public class CodeOwnersInternalServerErrorException extends RuntimeException {
private static final long serialVersionUID = 1L;
private static final String USER_MESSAGE = "Internal server in code-owners plugin";
- public CodeOwnersInternalServerErrorException(String message) {
+ /**
+ * Creates a {@link CodeOwnersInternalServerErrorException} to signal an internal server error
+ * caused by an issue in the code-owners plugin.
+ *
+ * @param message the exception message
+ * @return the created exception
+ */
+ public static CodeOwnersInternalServerErrorException newInternalServerError(String message) {
+ return new CodeOwnersInternalServerErrorException(message);
+ }
+
+ /**
+ * Creates a {@link RuntimeException} to signal an internal server error.
+ *
+ * <p>By default it is assumed that the internal server error is caused by an issue in the
+ * code-owners plugin and a {@link CodeOwnersInternalServerErrorException} is returned.
+ *
+ * <p>However for some known causes that are unrelated to code owners a {@link StorageException}
+ * is thrown. This is to avoid that the code-owners plugin is mistakenly assumed to be the cause
+ * of these errors.
+ *
+ * @param message the exception message
+ * @param cause the exception cause
+ * @return the created exception
+ */
+ public static RuntimeException newInternalServerError(String message, Throwable cause) {
+ if (isNonCodeOwnersCause(cause)) {
+ return new StorageException(message, cause);
+ }
+ return new CodeOwnersInternalServerErrorException(message, cause);
+ }
+
+ private CodeOwnersInternalServerErrorException(String message) {
super(message);
}
- public CodeOwnersInternalServerErrorException(String message, Throwable cause) {
+ private CodeOwnersInternalServerErrorException(String message, Throwable cause) {
super(message, cause);
}
public String getUserVisibleMessage() {
return USER_MESSAGE;
}
+
+ private static boolean isNonCodeOwnersCause(Throwable throwable) {
+ return hasCause(DiffNotAvailableException.class, throwable);
+ }
+
+ private static boolean hasCause(Class<?> exceptionClass, Throwable throwable) {
+ return Throwables.getCausalChain(throwable).stream().anyMatch(exceptionClass::isInstance);
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index a694ce5..ae1f477 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -23,7 +23,6 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.events.ReviewerAddedListener;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -44,7 +43,7 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.nio.file.Path;
-import java.sql.Timestamp;
+import java.time.Instant;
import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
@@ -147,7 +146,7 @@
Project.NameKey projectName,
Change.Id changeId,
List<AccountInfo> reviewers,
- Timestamp when,
+ Instant when,
int maxPathsInChangeMessages,
boolean asynchronous) {
try (Timer1.Context<String> ctx =
@@ -166,10 +165,17 @@
})
.call();
} catch (Exception e) {
- logger.atSevere().withCause(e).log(
- String.format(
- "Failed to post code-owners change message for reviewer on change %s in project %s.",
- changeId, projectName));
+ Optional<? extends Exception> configurationError =
+ CodeOwnersExceptionHook.getCauseOfConfigurationError(e);
+ if (configurationError.isPresent()) {
+ logger.atWarning().log(
+ "Failed to post code-owners change message for reviewer on change %s in project %s: %s",
+ changeId, projectName, configurationError.get().getMessage());
+ } else {
+ logger.atSevere().withCause(e).log(
+ "Failed to post code-owners change message for reviewer on change %s in project %s.",
+ changeId, projectName);
+ }
}
}
@@ -207,23 +213,15 @@
Project.NameKey projectName, Change.Id changeId, Account.Id reviewerAccountId) {
ChangeNotes changeNotes = changeNotesFactory.create(projectName, changeId);
- ImmutableList<Path> ownedPaths;
- try {
- // limit + 1, so that we can show an indicator if there are more than <limit> files.
- ownedPaths =
- OwnedChangedFile.getOwnedPaths(
- codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes,
- changeNotes.getCurrentPatchSet(),
- reviewerAccountId,
- /* start= */ 0,
- limit + 1));
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- "Couldn't compute owned paths of change %s for account %s",
- changeNotes.getChangeId(), reviewerAccountId.get());
- return Optional.empty();
- }
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
+ ImmutableList<Path> ownedPaths =
+ OwnedChangedFile.getOwnedPaths(
+ codeOwnerApprovalCheck.getOwnedPaths(
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ reviewerAccountId,
+ /* start= */ 0,
+ limit + 1));
if (ownedPaths.isEmpty()) {
// this reviewer doesn't own any of the modified paths
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/FallbackCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/FallbackCodeOwners.java
index 872473b..5ad4079 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/FallbackCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/FallbackCodeOwners.java
@@ -32,11 +32,5 @@
* with configuring code owners can easily happen. This is why this option is intended to be only
* used if requiring code owner approvals should not be enforced.
*/
- ALL_USERS,
-
- /**
- * Paths for which no code owners are defined are owned by the project owners. This means changes
- * to these paths can be approved by the project owners.
- */
- PROJECT_OWNERS;
+ ALL_USERS;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index 5b6f6dd..907f065 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -107,6 +106,23 @@
approvals,
requiredApproval,
maxPathsInChangeMessage);
+ } catch (Exception e) {
+ Optional<? extends Exception> configurationError =
+ CodeOwnersExceptionHook.getCauseOfConfigurationError(e);
+ if (configurationError.isPresent()) {
+ logger.atWarning().log(
+ "Failed to post code-owners change message for code owner approval on change %s"
+ + " in project %s: %s",
+ changeNotes.getChangeId(),
+ changeNotes.getProjectName(),
+ configurationError.get().getMessage());
+ } else {
+ logger.atSevere().withCause(e).log(
+ "Failed to post code-owners change message for code owner approval on change %s"
+ + " in project %s.",
+ changeNotes.getChangeId(), changeNotes.getProjectName());
+ }
+ return Optional.empty();
}
}
@@ -120,23 +136,15 @@
int limit) {
LabelVote newVote = getNewVote(requiredApproval, approvals);
- ImmutableList<Path> ownedPaths;
- try {
- // limit + 1, so that we can show an indicator if there are more than <limit> files.
- ownedPaths =
- OwnedChangedFile.getOwnedPaths(
- codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes,
- changeNotes.getCurrentPatchSet(),
- user.getAccountId(),
- /* start= */ 0,
- limit + 1));
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- "Couldn't compute owned paths of change %s for account %s",
- changeNotes.getChangeId(), user.getAccountId().get());
- return Optional.empty();
- }
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
+ ImmutableList<Path> ownedPaths =
+ OwnedChangedFile.getOwnedPaths(
+ codeOwnerApprovalCheck.getOwnedPaths(
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ user.getAccountId(),
+ /* start= */ 0,
+ limit + 1));
if (ownedPaths.isEmpty()) {
// the user doesn't own any of the modified paths
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
index e8a1ab7..b1a38fa 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
+
import com.google.auto.value.AutoValue;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
@@ -124,7 +126,7 @@
}
return Optional.of(ref.getObjectId());
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"failed to get revision of branch %s in project %s",
branchNameKey.shortName(), branchNameKey.project()),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/BackendConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/BackendConfig.java
index 05c9f0e..2077d64 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/BackendConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/BackendConfig.java
@@ -33,8 +33,6 @@
import com.google.gerrit.server.git.validators.ValidationMessage;
import com.google.inject.Inject;
import com.google.inject.Singleton;
-import java.util.ArrayList;
-import java.util.List;
import java.util.Optional;
import org.eclipse.jgit.lib.Config;
@@ -110,7 +108,7 @@
requireNonNull(fileName, "fileName");
requireNonNull(projectLevelConfig, "projectLevelConfig");
- List<CommitValidationMessage> validationMessages = new ArrayList<>();
+ ImmutableList.Builder<CommitValidationMessage> validationMessages = ImmutableList.builder();
String backendName = projectLevelConfig.getString(SECTION_CODE_OWNERS, null, KEY_BACKEND);
if (backendName != null) {
@@ -166,7 +164,7 @@
}
}
- return ImmutableList.copyOf(validationMessages);
+ return validationMessages.build();
}
/**
@@ -220,7 +218,7 @@
SECTION_CODE_OWNERS,
branch,
KEY_BACKEND));
- logger.atSevere().log(e.getMessage());
+ logger.atSevere().log("%s", e.getMessage());
return e;
}));
}
@@ -252,7 +250,7 @@
"Code owner backend '%s' that is configured for project %s in"
+ " %s.config (parameter %s.%s) not found.",
backendName, project, pluginName, SECTION_CODE_OWNERS, KEY_BACKEND));
- logger.atSevere().log(e.getMessage());
+ logger.atSevere().log("%s", e.getMessage());
return e;
}));
}
@@ -269,7 +267,7 @@
"Code owner backend '%s' that is configured in gerrit.config"
+ " (parameter plugin.%s.%s) not found.",
defaultBackendName, pluginName, KEY_BACKEND));
- logger.atSevere().log(e.getMessage());
+ logger.atSevere().log("%s", e.getMessage());
return e;
});
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
index 97029e0..df8003f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.codeowners.backend.config;
+import static com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS;
+
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.RefNames;
@@ -33,8 +35,6 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -79,38 +79,95 @@
@Override
public ImmutableList<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
throws CommitValidationException {
- String fileName = pluginName + ".config";
+ if (!receiveEvent.refName.equals(RefNames.REFS_CONFIG)) {
+ // The code-owners.config file is stored in refs/meta/config, if refs/meta/config was not
+ // modified we do not need to do any validation and can return early.
+ return ImmutableList.of();
+ }
+ ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder =
+ ImmutableList.builder();
+ validationMessageBuilder.addAll(
+ getWarningsForIgnoredCodeOwnerConfigurationInProjectConfig(receiveEvent));
+ String codeOwnersConfigFileName = pluginName + ".config";
try {
- if (!receiveEvent.refName.equals(RefNames.REFS_CONFIG)
- || !isFileChanged(receiveEvent, fileName)) {
+ if (!isFileChanged(receiveEvent, codeOwnersConfigFileName)) {
// the code-owners.config file in refs/meta/config was not modified, hence we do not need to
// validate it
- return ImmutableList.of();
+ return validationMessageBuilder.build();
}
ProjectState projectState = getProjectState(receiveEvent);
- ProjectLevelConfig.Bare cfg = loadConfig(receiveEvent, fileName);
- ImmutableList<CommitValidationMessage> validationMessages =
- validateConfig(projectState, fileName, cfg.getConfig());
+ ProjectLevelConfig.Bare cfg = loadConfig(receiveEvent, codeOwnersConfigFileName);
+ validationMessageBuilder.addAll(
+ validateConfig(projectState, codeOwnersConfigFileName, cfg.getConfig()));
+
+ ImmutableList<CommitValidationMessage> validationMessages = validationMessageBuilder.build();
if (!validationMessages.isEmpty()) {
throw new CommitValidationException(
- exceptionMessage(fileName, cfg.getRevision()), validationMessages);
+ exceptionMessage(codeOwnersConfigFileName, cfg.getRevision()), validationMessages);
}
return ImmutableList.of();
} catch (IOException | DiffNotAvailableException | ConfigInvalidException e) {
String errorMessage =
String.format(
"failed to validate file %s for revision %s in ref %s of project %s",
- fileName,
+ codeOwnersConfigFileName,
receiveEvent.commit.getName(),
RefNames.REFS_CONFIG,
receiveEvent.project.getNameKey());
- logger.atSevere().withCause(e).log(errorMessage);
+ logger.atSevere().withCause(e).log("%s", errorMessage);
throw new CommitValidationException(errorMessage, e);
}
}
+ private ImmutableList<CommitValidationMessage>
+ getWarningsForIgnoredCodeOwnerConfigurationInProjectConfig(CommitReceivedEvent receiveEvent) {
+ try {
+ if (!isFileChanged(receiveEvent, ProjectConfig.PROJECT_CONFIG)) {
+ return ImmutableList.of();
+ }
+
+ ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder =
+ ImmutableList.builder();
+ ProjectLevelConfig.Bare cfg = loadConfig(receiveEvent, ProjectConfig.PROJECT_CONFIG);
+
+ if (cfg.getConfig().getSubsections("plugin").contains(pluginName)) {
+ // The plugin.code-owners section is only read from gerrit.config, but not from
+ // project.config. Warn that this configuration is ignored and has no effect.
+ validationMessageBuilder.add(
+ new CommitValidationMessage(
+ String.format(
+ "Section 'plugin.code-owners' in %s is ignored and has no effect."
+ + " The configuration for the %s plugin must be done in %s.config.",
+ ProjectConfig.PROJECT_CONFIG, pluginName, pluginName),
+ ValidationMessage.Type.HINT));
+ }
+
+ if (cfg.getConfig().getSections().contains(SECTION_CODE_OWNERS)) {
+ // The codeOwners section is only read from code-owners.config, but not from
+ // project.config. Warn that this configuration is ignored and has no effect.
+ validationMessageBuilder.add(
+ new CommitValidationMessage(
+ String.format(
+ "Section 'codeOwners' in %s is ignored and has no effect."
+ + " The configuration for the %s plugin must be done in %s.config.",
+ ProjectConfig.PROJECT_CONFIG, pluginName, pluginName),
+ ValidationMessage.Type.HINT));
+ }
+
+ return validationMessageBuilder.build();
+ } catch (IOException | DiffNotAvailableException | CommitValidationException e) {
+ logger.atSevere().withCause(e).log(
+ "failed to inspect file %s for revision %s in ref %s of project %s",
+ ProjectConfig.PROJECT_CONFIG,
+ receiveEvent.commit.getName(),
+ RefNames.REFS_CONFIG,
+ receiveEvent.project.getNameKey());
+ return ImmutableList.of();
+ }
+ }
+
private ProjectState getProjectState(CommitReceivedEvent receiveEvent)
throws IOException, ConfigInvalidException {
ProjectConfig projectConfig = projectConfigFactory.create(receiveEvent.project.getNameKey());
@@ -166,7 +223,7 @@
*/
public ImmutableList<CommitValidationMessage> validateConfig(
ProjectState projectState, String fileName, Config cfg) {
- List<CommitValidationMessage> validationMessages = new ArrayList<>();
+ ImmutableList.Builder<CommitValidationMessage> validationMessages = ImmutableList.builder();
validationMessages.addAll(backendConfig.validateProjectLevelConfig(fileName, cfg));
validationMessages.addAll(generalConfig.validateProjectLevelConfig(fileName, cfg));
validationMessages.addAll(statusConfig.validateProjectLevelConfig(fileName, cfg));
@@ -174,7 +231,7 @@
requiredApprovalConfig.validateProjectLevelConfig(projectState, fileName, cfg));
validationMessages.addAll(
overrideApprovalConfig.validateProjectLevelConfig(projectState, fileName, cfg));
- return ImmutableList.copyOf(validationMessages);
+ return validationMessages.build();
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index 96e2685..5b2ab1c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend.config;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.util.Objects.requireNonNull;
@@ -30,7 +31,6 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.EnableImplicitApprovals;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
@@ -74,6 +74,9 @@
@Nullable private Boolean rejectNonResolvableImports;
@Nullable
+ private CodeOwnerConfigValidationPolicy codeOwnerConfigValidationPolicyForBranchCreation;
+
+ @Nullable
private CodeOwnerConfigValidationPolicy codeOwnerConfigValidationPolicyForCommitReceived;
@Nullable private CodeOwnerConfigValidationPolicy codeOwnerConfigValidationPolicyForSubmit;
@@ -204,6 +207,36 @@
}
/**
+ * Whether code owner configs should be validated when a branch is created.
+ *
+ * @param branchName the branch for which it should be checked whether code owner configs should
+ * be validated on branch creation
+ */
+ public CodeOwnerConfigValidationPolicy getCodeOwnerConfigValidationPolicyForBranchCreation(
+ String branchName) {
+ if (codeOwnerConfigValidationPolicyForBranchCreation == null) {
+ codeOwnerConfigValidationPolicyForBranchCreation =
+ readCodeOwnerConfigValidationPolicyForBranchCreation(branchName);
+ }
+ return codeOwnerConfigValidationPolicyForBranchCreation;
+ }
+
+ private CodeOwnerConfigValidationPolicy readCodeOwnerConfigValidationPolicyForBranchCreation(
+ String branchName) {
+ requireNonNull(branchName, "branchName");
+
+ Optional<CodeOwnerConfigValidationPolicy> branchSpecificPolicy =
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(projectName, branchName), pluginConfig);
+ if (branchSpecificPolicy.isPresent()) {
+ return branchSpecificPolicy.get();
+ }
+
+ return generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreation(
+ projectName, pluginConfig);
+ }
+
+ /**
* Whether code owner configs should be validated when a commit is received.
*
* @param branchName the branch for which it should be checked whether code owner configs should
@@ -332,7 +365,7 @@
return ImmutableSet.copyOf(exemptedAccounts.values());
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"Failed to resolve exempted users %s on project %s", exemptedUsers, projectName),
e);
@@ -662,7 +695,7 @@
filterOutDuplicateRequiredApprovals(
getConfiguredRequiredApproval(overrideApprovalConfig)));
} catch (InvalidPluginConfigurationException e) {
- logger.atWarning().withCause(e).log(
+ logger.atInfo().log(
"Ignoring invalid override approval configuration for project %s."
+ " Overrides are disabled.",
projectName.get());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
index 7e71f09..00574bc 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -38,9 +38,7 @@
import com.google.gerrit.server.project.RefPatternMatcher;
import com.google.inject.Inject;
import com.google.inject.Singleton;
-import java.util.ArrayList;
import java.util.Arrays;
-import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.regex.PatternSyntaxException;
@@ -72,6 +70,8 @@
public static final String KEY_READ_ONLY = "readOnly";
public static final String KEY_EXEMPT_PURE_REVERTS = "exemptPureReverts";
public static final String KEY_FALLBACK_CODE_OWNERS = "fallbackCodeOwners";
+ public static final String KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION =
+ "enableValidationOnBranchCreation";
public static final String KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED =
"enableValidationOnCommitReceived";
public static final String KEY_ENABLE_VALIDATION_ON_SUBMIT = "enableValidationOnSubmit";
@@ -113,7 +113,7 @@
requireNonNull(fileName, "fileName");
requireNonNull(projectLevelConfig, "projectLevelConfig");
- List<CommitValidationMessage> validationMessages = new ArrayList<>();
+ ImmutableList.Builder<CommitValidationMessage> validationMessages = ImmutableList.builder();
try {
projectLevelConfig.getEnum(
@@ -175,7 +175,7 @@
ValidationMessage.Type.ERROR));
}
- return ImmutableList.copyOf(validationMessages);
+ return validationMessages.build();
}
/**
@@ -475,6 +475,52 @@
}
/**
+ * Gets the enable validation on branch creation configuration from the given plugin config for
+ * the specified project with fallback to {@code gerrit.config} and default to {@code true}.
+ *
+ * <p>The enable validation on branch creation controls whether code owner config files should be
+ * validated when a branch is created.
+ *
+ * @param project the project for which the enable validation on branch creation configuration
+ * should be read
+ * @param pluginConfig the plugin config from which the enable validation on branch creation
+ * configuration should be read
+ * @return whether code owner config files should be validated when a branch is created
+ */
+ CodeOwnerConfigValidationPolicy getCodeOwnerConfigValidationPolicyForBranchCreation(
+ Project.NameKey project, Config pluginConfig) {
+ return getCodeOwnerConfigValidationPolicy(
+ KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ project,
+ pluginConfig,
+ CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ /**
+ * Gets the enable validation on branch creation configuration from the given plugin config for
+ * the specified branch.
+ *
+ * <p>If multiple branch-specific configurations match the specified branch, it is undefined which
+ * of the matching branch configurations takes precedence.
+ *
+ * <p>The enable validation on branch creation controls whether code owner config files should be
+ * validated when a branch is created.
+ *
+ * @param branchNameKey the branch and project for which the enable validation on branch creation
+ * configuration should be read
+ * @param pluginConfig the plugin config from which the enable validation on branch creation
+ * configuration should be read
+ * @return the enable validation on branch creation configuration that is configured for the
+ * branch, {@link Optional#empty()} if no branch specific configuration exists
+ */
+ Optional<CodeOwnerConfigValidationPolicy>
+ getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey branchNameKey, Config pluginConfig) {
+ return getCodeOwnerConfigValidationPolicyForBranch(
+ KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION, branchNameKey, pluginConfig);
+ }
+
+ /**
* Gets the enable validation on commit received configuration from the given plugin config for
* the specified project with fallback to {@code gerrit.config} and default to {@code true}.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/StatusConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/StatusConfig.java
index c3d4dee..d75a3fa 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/StatusConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/StatusConfig.java
@@ -31,8 +31,6 @@
import com.google.gerrit.server.project.RefPatternMatcher;
import com.google.inject.Inject;
import com.google.inject.Singleton;
-import java.util.ArrayList;
-import java.util.List;
import java.util.regex.PatternSyntaxException;
import org.eclipse.jgit.lib.Config;
@@ -81,7 +79,7 @@
requireNonNull(fileName, "fileName");
requireNonNull(projectLevelConfig, "projectLevelConfig");
- List<CommitValidationMessage> validationMessages = new ArrayList<>();
+ ImmutableList.Builder<CommitValidationMessage> validationMessages = ImmutableList.builder();
try {
projectLevelConfig.getBoolean(SECTION_CODE_OWNERS, null, KEY_DISABLED, false);
@@ -117,7 +115,7 @@
}
}
- return ImmutableList.copyOf(validationMessages);
+ return validationMessages.build();
}
/**
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 01ab9b9..5f35099 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
@@ -191,7 +191,7 @@
private static final Pattern PAT_PER_FILE =
Pattern.compile(BOL + "per-file[\\s]+([^=#]+)=[\\s]*([^#]+)" + EOL);
- private List<ValidationError> validationErrors;
+ private ImmutableList.Builder<ValidationError> validationErrors;
CodeOwnerConfig parse(
ObjectId revision, CodeOwnerConfig.Key codeOwnerConfigKey, String codeOwnerConfigAsString) {
@@ -264,12 +264,12 @@
new String[] {
removeExtraSpaces(perFileMatcher.group(1)), removeExtraSpaces(perFileMatcher.group(2))
};
- String[] dirGlobs = splitGlobs(globsAndOwners[0]);
+ ImmutableSet<String> dirGlobs = splitGlobs(globsAndOwners[0]);
String directive = globsAndOwners[1];
if (directive.equals(TOK_SET_NOPARENT)) {
return CodeOwnerSet.builder()
.setIgnoreGlobalAndParentCodeOwners()
- .setPathExpressions(ImmutableSet.copyOf(dirGlobs))
+ .setPathExpressions(dirGlobs)
.build();
}
@@ -277,7 +277,7 @@
if ((codeOwnerConfigReference = parseInclude(directive)) != null) {
return CodeOwnerSet.builder()
.addImport(codeOwnerConfigReference)
- .setPathExpressions(ImmutableSet.copyOf(dirGlobs))
+ .setPathExpressions(dirGlobs)
.build();
}
@@ -296,7 +296,7 @@
CodeOwnerSet.Builder codeOwnerSet =
CodeOwnerSet.builder()
- .setPathExpressions(ImmutableSet.copyOf(dirGlobs))
+ .setPathExpressions(dirGlobs)
.setCodeOwners(
ownerEmails.stream().map(CodeOwnerReference::create).collect(toImmutableSet()));
ownerEmails.stream()
@@ -320,8 +320,8 @@
* @return the globs as array
*/
@VisibleForTesting
- static String[] splitGlobs(String commaSeparatedGlobs) {
- ArrayList<String> globList = new ArrayList<>();
+ static ImmutableSet<String> splitGlobs(String commaSeparatedGlobs) {
+ ImmutableSet.Builder<String> globList = ImmutableSet.builder();
StringBuilder nextGlob = new StringBuilder();
int curlyBracesIndentionLevel = 0;
int squareBracesIndentionLevel = 0;
@@ -354,7 +354,7 @@
if (nextGlob.length() > 0) {
globList.add(nextGlob.toString());
}
- return globList.toArray(new String[globList.size()]);
+ return globList.build();
}
private static boolean isComment(String line) {
@@ -433,7 +433,7 @@
*/
public ImmutableList<ValidationError> getValidationErrors() {
if (validationErrors != null) {
- return ImmutableList.copyOf(validationErrors);
+ return validationErrors.build();
}
return ImmutableList.of();
}
@@ -441,7 +441,7 @@
@Override
public void error(ValidationError error) {
if (validationErrors == null) {
- validationErrors = new ArrayList<>(4);
+ validationErrors = ImmutableList.builder();
}
validationErrors.add(error);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/ValidationTrigger.java b/java/com/google/gerrit/plugins/codeowners/metrics/ValidationTrigger.java
index 4d28b70..8fc24d0 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/ValidationTrigger.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/ValidationTrigger.java
@@ -16,6 +16,9 @@
/** Enum to express which event triggered the validation. */
public enum ValidationTrigger {
+ /** A new branch is created for which the initial commit should be validated. */
+ BRANCH_CREATION,
+
/** A new commit was received that should be validated. */
COMMIT_RECEIVED,
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 56aa627..677e67c 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -18,6 +18,7 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_EXPLICITLY_MENTIONED_SCORING_VALUE;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NOT_EXPLICITLY_MENTIONED_SCORING_VALUE;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
@@ -46,7 +47,6 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScorings;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.account.AccountControl;
@@ -580,7 +580,7 @@
throw new IllegalStateException("unknown account visibility setting: " + accountVisibility);
} catch (IOException | PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException("failed to get visible users", e);
+ throw newInternalServerError("failed to get visible users", e);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractPathResource.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractPathResource.java
index 75cc0eb..dd603fc 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractPathResource.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractPathResource.java
@@ -14,11 +14,10 @@
package com.google.gerrit.plugins.codeowners.restapi;
-import static com.google.common.base.Preconditions.checkState;
-
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestResource;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.project.BranchResource;
@@ -58,16 +57,14 @@
private final ObjectId revision;
private final Path path;
- protected AbstractPathResource(BranchResource branchResource, Path path) {
+ protected AbstractPathResource(BranchResource branchResource, Path path)
+ throws ResourceNotFoundException {
+ if (branchResource.getRevision().isEmpty()) {
+ throw new ResourceNotFoundException(IdString.fromDecoded(branchResource.getName()));
+ }
+
this.branchNameKey = branchResource.getBranchKey();
-
- checkState(
- branchResource.getRevision() != null,
- "branch %s in project %s wasn't created yet",
- branchResource.getBranchKey().branch(),
- branchResource.getBranchKey().project().get());
- this.revision = ObjectId.fromString(branchResource.getRevision());
-
+ this.revision = ObjectId.fromString(branchResource.getRevision().get());
this.path = path;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
index 9c4e144..ad8b388 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -36,7 +36,6 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwners;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.backend.OptionalResultWithMessages;
import com.google.gerrit.plugins.codeowners.backend.PathCodeOwners;
@@ -46,14 +45,12 @@
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.BranchResource;
import com.google.gerrit.server.restapi.account.AccountsCollection;
@@ -151,7 +148,7 @@
@Override
public Response<CodeOwnerCheckInfo> apply(BranchResource branchResource)
throws BadRequestException, AuthException, IOException, ConfigInvalidException,
- PermissionBackendException {
+ PermissionBackendException, ResourceNotFoundException {
permissionBackend.currentUser().check(checkCodeOwnerCapability.getPermission());
validateInput(branchResource);
@@ -167,7 +164,7 @@
Set<String> annotations = new HashSet<>();
codeOwnerConfigHierarchy.visit(
branchResource.getBranchKey(),
- ObjectId.fromString(branchResource.getRevision()),
+ ObjectId.fromString(branchResource.getRevision().get()),
absolutePath,
codeOwnerConfig -> {
messages.add(
@@ -351,7 +348,11 @@
private void validateInput(BranchResource branchResource)
throws BadRequestException, AuthException, IOException, ConfigInvalidException,
- PermissionBackendException {
+ PermissionBackendException, ResourceNotFoundException {
+ if (branchResource.getRevision().isEmpty()) {
+ throw new ResourceNotFoundException(IdString.fromDecoded(branchResource.getName()));
+ }
+
if (email == null) {
throw new BadRequestException("email required");
}
@@ -399,8 +400,6 @@
switch (fallbackCodeOwners) {
case NONE:
return false;
- case PROJECT_OWNERS:
- return isProjectOwner(projectName);
case ALL_USERS:
return true;
}
@@ -410,26 +409,6 @@
fallbackCodeOwners.name(), projectName));
}
- private boolean isProjectOwner(Project.NameKey projectName) {
- try {
- AccountResource accountResource =
- accountsCollection.parse(TopLevelResource.INSTANCE, IdString.fromDecoded(email));
- // There is no dedicated project owner permission, but project owners are detected by checking
- // the permission to write the project config. Only project owners can do this.
- return permissionBackend
- .absentUser(accountResource.getUser().getAccountId())
- .project(projectName)
- .test(ProjectPermission.WRITE_CONFIG);
- } catch (PermissionBackendException
- | ResourceNotFoundException
- | AuthException
- | IOException
- | ConfigInvalidException e) {
- throw new CodeOwnersInternalServerErrorException(
- String.format("failed if email %s is owner of project %s", email, projectName.get()), e);
- }
- }
-
private OptionalResultWithMessages<CodeOwner> isResolvable() {
CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get();
if (identifiedUser != null) {
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerConfigsInBranchCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerConfigsInBranchCollection.java
index 2ed2706..3d324d5 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerConfigsInBranchCollection.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerConfigsInBranchCollection.java
@@ -49,7 +49,8 @@
}
@Override
- public PathResource parse(BranchResource branchResource, IdString id) throws BadRequestException {
+ public PathResource parse(BranchResource branchResource, IdString id)
+ throws BadRequestException, ResourceNotFoundException {
return PathResource.parse(branchResource, id);
}
@@ -69,15 +70,15 @@
* The resource kind of the members in the {@link CodeOwnerConfigsInBranchCollection} REST
* collection.
*/
- static final TypeLiteral<RestView<PathResource>> PATH_KIND =
- new TypeLiteral<RestView<PathResource>>() {};
+ static final TypeLiteral<RestView<PathResource>> PATH_KIND = new TypeLiteral<>() {};
static PathResource parse(BranchResource branchResource, IdString pathId)
- throws BadRequestException {
+ throws BadRequestException, ResourceNotFoundException {
return new PathResource(branchResource, parsePath(pathId));
}
- private PathResource(BranchResource branchResource, Path path) {
+ private PathResource(BranchResource branchResource, Path path)
+ throws ResourceNotFoundException {
super(branchResource, path);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInBranchCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInBranchCollection.java
index 7b0f6c3..feb1199 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInBranchCollection.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInBranchCollection.java
@@ -48,7 +48,8 @@
}
@Override
- public PathResource parse(BranchResource branchResource, IdString id) throws BadRequestException {
+ public PathResource parse(BranchResource branchResource, IdString id)
+ throws BadRequestException, ResourceNotFoundException {
return PathResource.parse(branchResource, id);
}
@@ -67,11 +68,10 @@
/**
* The resource kind of the members in the {@link CodeOwnersInBranchCollection} REST collection.
*/
- static final TypeLiteral<RestView<PathResource>> PATH_KIND =
- new TypeLiteral<RestView<PathResource>>() {};
+ static final TypeLiteral<RestView<PathResource>> PATH_KIND = new TypeLiteral<>() {};
static PathResource parse(BranchResource branchResource, IdString pathId)
- throws BadRequestException {
+ throws BadRequestException, ResourceNotFoundException {
return new PathResource(branchResource, parsePath(pathId));
}
@@ -79,7 +79,8 @@
return new PathResource(getBranch(), revision, getPath());
}
- private PathResource(BranchResource branchResource, Path path) {
+ private PathResource(BranchResource branchResource, Path path)
+ throws ResourceNotFoundException {
super(branchResource, path);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
index 20e8dfa..642d605 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
@@ -135,8 +135,7 @@
/**
* The resource kind of the members in the {@link CodeOwnersInChangeCollection} REST collection.
*/
- static final TypeLiteral<RestView<PathResource>> PATH_KIND =
- new TypeLiteral<RestView<PathResource>>() {};
+ static final TypeLiteral<RestView<PathResource>> PATH_KIND = new TypeLiteral<>() {};
static PathResource parse(
RevisionResource revisionResource, ObjectId branchRevision, IdString pathId)
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java b/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java
index 96b467e..21462a2 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_SUBMIT;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_EXEMPTED_USER;
@@ -221,6 +222,14 @@
input.exemptPureReverts);
}
+ if (input.enableValidationOnBranchCreation != null) {
+ codeOwnersConfig.setEnum(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ input.enableValidationOnBranchCreation);
+ }
+
if (input.enableValidationOnCommitReceived != null) {
codeOwnersConfig.setEnum(
SECTION_CODE_OWNERS,
@@ -264,7 +273,7 @@
validateConfig(projectResource.getProjectState(), codeOwnersConfig);
codeOwnersProjectConfigFile.commit(metaDataUpdate);
- projectCache.evict(projectResource.getNameKey());
+ projectCache.evictAndReindex(projectResource.getNameKey());
}
CodeOwnerProjectConfigInfo updatedCodeOwnerProjectConfigInfo =
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 136ffcb..f763a2c 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -18,6 +18,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidCodeOwnerConfigCause;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.auto.value.AutoValue;
@@ -53,6 +54,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.events.CommitReceivedEvent;
+import com.google.gerrit.server.events.RefReceivedEvent;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -61,6 +63,7 @@
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.MergeValidationException;
import com.google.gerrit.server.git.validators.MergeValidationListener;
+import com.google.gerrit.server.git.validators.RefOperationValidationListener;
import com.google.gerrit.server.git.validators.ValidationMessage;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -73,6 +76,7 @@
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -84,10 +88,13 @@
import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
+import org.eclipse.jgit.treewalk.TreeWalk;
/**
* Validates modifications to the code owner config files.
@@ -128,7 +135,8 @@
* </ul>
*/
@Singleton
-public class CodeOwnerConfigValidator implements CommitValidationListener, MergeValidationListener {
+public class CodeOwnerConfigValidator
+ implements CommitValidationListener, MergeValidationListener, RefOperationValidationListener {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final String pluginName;
@@ -207,6 +215,7 @@
receiveEvent.commit,
receiveEvent.user,
codeOwnerConfigValidationPolicy.isForced(),
+ /* isBranchCreation= */ false,
receiveEvent.pushOptions);
} catch (RuntimeException e) {
codeOwnerMetrics.countCodeOwnerConfigValidations.increment(
@@ -295,6 +304,7 @@
commit,
patchSetUploader,
codeOwnerConfigValidationPolicy.isForced(),
+ /* isBranchCreation= */ false,
/* pushOptions= */ ImmutableListMultimap.of());
} catch (RuntimeException e) {
codeOwnerMetrics.countCodeOwnerConfigValidations.increment(
@@ -328,6 +338,101 @@
}
}
+ @Override
+ public List<ValidationMessage> onRefOperation(RefReceivedEvent refReceivedEvent)
+ throws ValidationException {
+ if (!ReceiveCommand.Type.CREATE.equals(refReceivedEvent.command.getType())) {
+ // We are only interested in branch creations. Return early if this is not a branch creation.
+ return ImmutableList.of();
+ }
+
+ try (TraceTimer traceTimer =
+ TraceContext.newTimer(
+ "Validate code owner config files on branch creation",
+ Metadata.builder()
+ .projectName(refReceivedEvent.getProjectNameKey().get())
+ .commit(refReceivedEvent.command.getNewId().name())
+ .branchName(refReceivedEvent.getRefName())
+ .username(refReceivedEvent.user.getLoggableName())
+ .build())) {
+ CodeOwnerConfigValidationPolicy codeOwnerConfigValidationPolicy =
+ codeOwnersPluginConfiguration
+ .getProjectConfig(refReceivedEvent.getProjectNameKey())
+ .getCodeOwnerConfigValidationPolicyForBranchCreation(refReceivedEvent.getRefName());
+ logger.atFine().log("codeOwnerConfigValidationPolicy = %s", codeOwnerConfigValidationPolicy);
+ boolean metricRecordingDone = false;
+ Optional<ValidationResult> validationResult;
+ if (!codeOwnerConfigValidationPolicy.runValidation()) {
+ validationResult =
+ Optional.of(
+ ValidationResult.create(
+ pluginName,
+ "skipping validation of code owner config files",
+ new CommitValidationMessage(
+ "code owners config validation is disabled", ValidationMessage.Type.HINT)));
+ } else {
+ try {
+ try (Repository repo = repoManager.openRepository(refReceivedEvent.getProjectNameKey());
+ RevWalk revWalk = new RevWalk(repo)) {
+ validationResult =
+ validateCodeOwnerConfig(
+ refReceivedEvent.getBranchNameKey(),
+ revWalk.parseCommit(refReceivedEvent.command.getNewId()),
+ refReceivedEvent.user,
+ codeOwnerConfigValidationPolicy.isForced(),
+ /* isBranchCreation= */ true,
+ refReceivedEvent.pushOptions);
+ } catch (IOException e) {
+ throw newInternalServerError(
+ String.format(
+ "failed to validate code owner config files in revision %s"
+ + " (project = %s, branch = %s)",
+ refReceivedEvent.command.getNewId().name(),
+ refReceivedEvent.getProjectNameKey(),
+ refReceivedEvent.getRefName()),
+ e);
+ }
+ } catch (RuntimeException e) {
+ codeOwnerMetrics.countCodeOwnerConfigValidations.increment(
+ ValidationTrigger.BRANCH_CREATION,
+ com.google.gerrit.plugins.codeowners.metrics.ValidationResult.FAILED,
+ codeOwnerConfigValidationPolicy.isDryRun());
+ metricRecordingDone = true;
+
+ if (!codeOwnerConfigValidationPolicy.isDryRun()) {
+ throw e;
+ }
+
+ // The validation was executed as dry-run and failures during the validation should not
+ // cause an error. Hence we swallow the exception here.
+ logger.atWarning().withCause(e).log(
+ "ignoring failure during validation of code owner config files in revision %s"
+ + " (project = %s, branch = %s) because the validation was performed as dry-run",
+ refReceivedEvent.command.getNewId().getName(),
+ refReceivedEvent.getProjectNameKey(),
+ refReceivedEvent.getBranchNameKey().branch());
+ validationResult = Optional.empty();
+ }
+ }
+ if (!validationResult.isPresent()) {
+ return ImmutableList.of();
+ }
+
+ logger.atFine().log("validation result = %s", validationResult.get());
+ if (!metricRecordingDone) {
+ codeOwnerMetrics.countCodeOwnerConfigValidations.increment(
+ ValidationTrigger.BRANCH_CREATION,
+ validationResult.get().hasError()
+ ? com.google.gerrit.plugins.codeowners.metrics.ValidationResult.REJECTED
+ : com.google.gerrit.plugins.codeowners.metrics.ValidationResult.PASSED,
+ codeOwnerConfigValidationPolicy.isDryRun());
+ }
+ return validationResult
+ .get()
+ .processForOnRefOperation(codeOwnerConfigValidationPolicy.isDryRun());
+ }
+ }
+
/**
* Validates the code owner config files which are newly added or modified in the given commit.
*
@@ -338,6 +443,7 @@
* @param user user for which the code owner visibility checks should be performed
* @param force whether the validation should be done even if the code owners functionality is
* disabled for the branch
+ * @param isBranchCreation whether a new branch is being created
* @return the validation result, {@link Optional#empty()} if no validation is performed because
* the given commit doesn't contain newly added or modified code owner configs
*/
@@ -346,6 +452,7 @@
RevCommit revCommit,
IdentifiedUser user,
boolean force,
+ boolean isBranchCreation,
ImmutableListMultimap<String, String> pushOptions) {
CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(branchNameKey.project());
@@ -380,7 +487,7 @@
"skipping code owner config validation not allowed",
new CommitValidationMessage(e.getMessage(), ValidationMessage.Type.ERROR)));
} catch (SkipCodeOwnerConfigValidationPushOption.InvalidValueException e) {
- logger.atFine().log(e.getMessage());
+ logger.atFine().log("%s", e.getMessage());
return Optional.of(
ValidationResult.create(
pluginName,
@@ -401,31 +508,13 @@
try {
CodeOwnerBackend codeOwnerBackend = codeOwnersConfig.getBackend(branchNameKey.branch());
- // For merge commits, always do the comparison against the destination branch
- // (MergeCommitStrategy.ALL_CHANGED_FILES). Doing the comparison against the auto-merge
- // (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION) is not possible because loading the
- // auto-merge cannot reuse the rev walk that can see newly created merge commits and hence
- // trying to get the auto merge would fail with a missing object exception. This is why we
- // use MergeCommitStrategy.ALL_CHANGED_FILES here even if
- // MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION is configured.
- ImmutableList<ChangedFile> modifiedCodeOwnerConfigFiles =
- changedFiles
- .getFromDiffCache(
- branchNameKey.project(), revCommit, MergeCommitStrategy.ALL_CHANGED_FILES)
- .stream()
- // filter out deletions (files without new path)
- .filter(changedFile -> changedFile.newPath().isPresent())
- // filter out non code owner config files
- .filter(
- changedFile ->
- codeOwnerBackend.isCodeOwnerConfigFile(
- branchNameKey.project(),
- Paths.get(changedFile.newPath().get().toString())
- .getFileName()
- .toString()))
- .collect(toImmutableList());
+ ImmutableList<ChangedFile> codeOwnerConfigFilesToValidate =
+ isBranchCreation
+ ? getAllCodeOwnerConfigFiles(codeOwnerBackend, branchNameKey.project(), revCommit)
+ : getModifiedCodeOwnerConfigFiles(
+ codeOwnerBackend, branchNameKey.project(), revCommit);
- if (modifiedCodeOwnerConfigFiles.isEmpty()) {
+ if (codeOwnerConfigFilesToValidate.isEmpty()) {
return Optional.empty();
}
@@ -433,7 +522,7 @@
return Optional.of(
ValidationResult.create(
pluginName,
- modifiedCodeOwnerConfigFiles.stream()
+ codeOwnerConfigFilesToValidate.stream()
.flatMap(
changedFile ->
validateCodeOwnerConfig(
@@ -445,10 +534,9 @@
// accept that it's possible to add invalid code owner configs while the plugin configuration
// is invalid.
logger.atWarning().log(
- String.format(
- "cannot validate code owner config files due to invalid code-owners plugin"
- + " configuration: %s",
- e.getMessage()));
+ "cannot validate code owner config files due to invalid code-owners plugin"
+ + " configuration: %s",
+ e.getMessage());
return Optional.of(
ValidationResult.create(
pluginName,
@@ -463,8 +551,49 @@
"failed to validate code owner config files in revision %s"
+ " (project = %s, branch = %s)",
revCommit.getName(), branchNameKey.project(), branchNameKey.branch());
- logger.atSevere().withCause(e).log(errorMessage);
- throw new CodeOwnersInternalServerErrorException(errorMessage, e);
+ throw newInternalServerError(errorMessage, e);
+ }
+ }
+
+ public ImmutableList<ChangedFile> getModifiedCodeOwnerConfigFiles(
+ CodeOwnerBackend codeOwnerBackend, Project.NameKey project, ObjectId revCommit)
+ throws IOException, DiffNotAvailableException {
+ // For merge commits, always do the comparison against the destination branch
+ // (MergeCommitStrategy.ALL_CHANGED_FILES). Doing the comparison against the auto-merge
+ // (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION) is not possible because loading the
+ // auto-merge cannot reuse the rev walk that can see newly created merge commits and hence
+ // trying to get the auto merge would fail with a missing object exception. This is why we
+ // use MergeCommitStrategy.ALL_CHANGED_FILES here even if
+ // MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION is configured.
+ return changedFiles.getFromDiffCache(project, revCommit, MergeCommitStrategy.ALL_CHANGED_FILES)
+ .stream()
+ // filter out deletions (files without new path)
+ .filter(changedFile -> changedFile.newPath().isPresent())
+ // filter out non code owner config files
+ .filter(
+ changedFile ->
+ codeOwnerBackend.isCodeOwnerConfigFile(
+ project,
+ Paths.get(changedFile.newPath().get().toString()).getFileName().toString()))
+ .collect(toImmutableList());
+ }
+
+ public ImmutableList<ChangedFile> getAllCodeOwnerConfigFiles(
+ CodeOwnerBackend codeOwnerBackend, Project.NameKey project, RevCommit revCommit)
+ throws IOException {
+ try (Repository git = repoManager.openRepository(project);
+ ObjectReader or = git.newObjectReader();
+ TreeWalk tw = new TreeWalk(or)) {
+ tw.addTree(revCommit.getTree());
+ tw.setRecursive(true);
+ ImmutableList.Builder<ChangedFile> paths = ImmutableList.builder();
+ while (tw.next()) {
+ Path path = Path.of(tw.getPathString());
+ if (codeOwnerBackend.isCodeOwnerConfigFile(project, path.getFileName().toString())) {
+ paths.add(ChangedFile.addition(path));
+ }
+ }
+ return paths.build();
}
}
@@ -893,7 +1022,7 @@
.filter(Optional::isPresent)
.map(Optional::get);
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("Failed to validate imports for %s in ", codeOwnerConfig.key()), e);
}
}
@@ -1063,7 +1192,7 @@
.project(keyOfImportedCodeOwnerConfig.project())
.test(ProjectPermission.ACCESS);
} catch (PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
"failed to check read permission for project of imported code owner config", e);
}
}
@@ -1076,7 +1205,7 @@
.ref(keyOfImportedCodeOwnerConfig.ref())
.test(RefPermission.READ);
} catch (PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
"failed to check read permission for branch of imported code owner config", e);
}
}
@@ -1097,8 +1226,7 @@
return Optional.ofNullable(repo.exactRef(keyOfImportedCodeOwnerConfig.ref()))
.map(Ref::getObjectId);
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
- "failed to read revision of import code owner config", e);
+ throw newInternalServerError("failed to read revision of import code owner config", e);
}
}
@@ -1223,7 +1351,7 @@
withPluginName(summaryMessage()), withPluginName(validationMessages()));
}
- return validationMessagesWithIncludedSummaryMessage();
+ return commitValidationMessagesWithIncludedSummaryMessage();
}
/**
@@ -1242,12 +1370,29 @@
if (!validationMessages().isEmpty()) {
logger.atFine().log(
"submitting changes to code owner config files with the following messages: %s",
- validationMessagesWithIncludedSummaryMessage());
+ commitValidationMessagesWithIncludedSummaryMessage());
} else {
logger.atFine().log("submitting changes to code owner config files, no issues found");
}
}
+ /**
+ * Processes the validation messages for a validation that is done when a ref operation is done
+ * (e.g. on branch creation).
+ *
+ * <p>Throws a {@link ValidationException} if there are errors to make the ref operation fail.
+ *
+ * <p>If there are no errors the validation messages are returned so that they can be sent to
+ * the client without causing the ref operation to fail.
+ */
+ List<ValidationMessage> processForOnRefOperation(boolean dryRun) throws ValidationException {
+ if (!dryRun && hasError()) {
+ throw new ValidationException(getMessage(validationMessages()));
+ }
+
+ return validationMessagesWithIncludedSummaryMessage();
+ }
+
/** Checks whether any of the validation messages is an error. */
public boolean hasError() {
return validationMessages().stream()
@@ -1257,7 +1402,8 @@
|| ValidationMessage.Type.ERROR.equals(validationMessage.getType()));
}
- private ImmutableList<CommitValidationMessage> validationMessagesWithIncludedSummaryMessage() {
+ private ImmutableList<CommitValidationMessage>
+ commitValidationMessagesWithIncludedSummaryMessage() {
return ImmutableList.<CommitValidationMessage>builder()
.add(
new CommitValidationMessage(
@@ -1266,6 +1412,15 @@
.build();
}
+ private ImmutableList<ValidationMessage> validationMessagesWithIncludedSummaryMessage() {
+ return ImmutableList.<ValidationMessage>builder()
+ .add(
+ new ValidationMessage(
+ withPluginName(summaryMessage()), getValidationMessageTypeForSummaryMessage()))
+ .addAll(withPluginName(validationMessages()))
+ .build();
+ }
+
/**
* Gets the validation message type that should be used for the summary message.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
index bc7ac6b..4417295 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
@@ -14,11 +14,12 @@
package com.google.gerrit.plugins.codeowners.validation;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
+
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.server.git.receive.PluginPushOption;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -103,7 +104,7 @@
.currentUser()
.check(skipCodeOwnerConfigValidationCapability.getPermission());
} catch (PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"Failed to check %s capability", SkipCodeOwnerConfigValidationCapability.ID),
e);
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/ValidationModule.java b/java/com/google/gerrit/plugins/codeowners/validation/ValidationModule.java
index d28925c..6f5ee08 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/ValidationModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/ValidationModule.java
@@ -20,6 +20,7 @@
import com.google.gerrit.server.git.receive.PluginPushOption;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
+import com.google.gerrit.server.git.validators.RefOperationValidationListener;
import com.google.inject.AbstractModule;
/** Guice module that registers validation extensions of the code-owners plugin. */
@@ -28,6 +29,8 @@
protected void configure() {
DynamicSet.bind(binder(), CommitValidationListener.class).to(CodeOwnerConfigValidator.class);
DynamicSet.bind(binder(), MergeValidationListener.class).to(CodeOwnerConfigValidator.class);
+ DynamicSet.bind(binder(), RefOperationValidationListener.class)
+ .to(CodeOwnerConfigValidator.class);
bind(CapabilityDefinition.class)
.annotatedWith(Exports.named(SkipCodeOwnerConfigValidationCapability.ID))
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
index cedced9..83eb13f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -24,6 +24,7 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
@@ -62,6 +63,7 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.Arrays;
+import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Test;
@@ -88,6 +90,25 @@
}
@Test
+ public void checkCodeOwnerForNonExistingBranch() throws Exception {
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/non-existing/code_owners.check", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
+ public void checkCodeOwnerForSymbolicRefPointingToAnUnbornBranch() throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ repo.updateRef(Constants.HEAD, true).link("refs/heads/non-existing");
+ }
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/HEAD/code_owners.check", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
public void requiresEmail() throws Exception {
BadRequestException exception =
assertThrows(BadRequestException.class, () -> checkCodeOwner("/", /* email= */ null));
@@ -1216,47 +1237,6 @@
}
@Test
- @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
- public void checkFallbackCodeOwner_ProjectOwners() throws Exception {
- TestAccount codeOwner =
- accountCreator.create(
- "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
- setAsCodeOwners("/foo/", codeOwner);
-
- // 1. Check for a file to which fallback code owners do not apply because code owners are
- // defined
- String path = "/foo/bar/baz.md";
-
- // 1a. by a code owner
- CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, codeOwner.email());
- assertThat(checkCodeOwnerInfo).isCodeOwner();
- assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
-
- // 1b. by a project owner
- checkCodeOwnerInfo = checkCodeOwner(path, admin.email());
- assertThat(checkCodeOwnerInfo).isNotCodeOwner();
- assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
-
- // 1c. by a non code owner
- checkCodeOwnerInfo = checkCodeOwner(path, user.email());
- assertThat(checkCodeOwnerInfo).isNotCodeOwner();
- assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
-
- // 2. Check for a file to which fallback code owners apply because no code owners are defined
- path = "/other/bar/baz.md";
-
- // 2b. by a project owner
- checkCodeOwnerInfo = checkCodeOwner(path, admin.email());
- assertThat(checkCodeOwnerInfo).isCodeOwner();
- assertThat(checkCodeOwnerInfo).isFallbackCodeOwner();
-
- // 2b. by a non project owner
- checkCodeOwnerInfo = checkCodeOwner(path, user.email());
- assertThat(checkCodeOwnerInfo).isNotCodeOwner();
- assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
- }
-
- @Test
@GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
public void noFallbackCodeOwnerIfParentCodeOwnersIgnored() throws Exception {
codeOwnerConfigOperations
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorErrorHandlingIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorErrorHandlingIT.java
index a1c34ba..28381f3 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorErrorHandlingIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorErrorHandlingIT.java
@@ -73,7 +73,7 @@
@Test
@GerritConfig(name = "plugin.code-owners.backend", value = FailingCodeOwnerBackend.ID)
- @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
@GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "true")
public void submitFailsOnInternalError() throws Exception {
try (AutoCloseable registration = registerTestBackend(new FailingCodeOwnerBackend())) {
@@ -93,7 +93,7 @@
@Test
@GerritConfig(name = "plugin.code-owners.backend", value = FailingCodeOwnerBackend.ID)
@GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "DRY_RUN")
- @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
public void submitSucceedsOnInternalErrorIfValidationIsDoneAsDryRun() throws Exception {
try (AutoCloseable registration = registerTestBackend(new FailingCodeOwnerBackend())) {
disableCodeOwnersForProject(project);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
index efa94d0..5b7ab3d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -16,6 +16,10 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
+import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
+import static com.google.gerrit.acceptance.GitUtil.assertPushRejected;
+import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
@@ -33,10 +37,14 @@
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.changes.CherryPickInput;
+import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.ProjectState;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.extensions.common.MergeInput;
import com.google.gerrit.extensions.restapi.MergeConflictException;
@@ -59,12 +67,15 @@
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
import com.google.gerrit.plugins.codeowners.validation.SkipCodeOwnerConfigValidationCapability;
import com.google.gerrit.plugins.codeowners.validation.SkipCodeOwnerConfigValidationPushOption;
+import com.google.gerrit.server.submit.IntegrationConflictException;
import com.google.inject.Inject;
+import java.nio.file.Path;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.PushResult;
import org.junit.Before;
import org.junit.Test;
@@ -1764,6 +1775,50 @@
}
@Test
+ public void cannotUploadConfigWithGlobalImportOfRootFolder() throws Exception {
+ testUploadConfigWithImportOfRootFolder(CodeOwnerConfigImportType.GLOBAL);
+ }
+
+ @Test
+ public void cannotUploadConfigWithPerFileImportOfRootFolder() throws Exception {
+ testUploadConfigWithImportOfRootFolder(CodeOwnerConfigImportType.PER_FILE);
+ }
+
+ private void testUploadConfigWithImportOfRootFolder(CodeOwnerConfigImportType importType)
+ throws Exception {
+ skipTestIfImportsNotSupportedByCodeOwnersBackend();
+
+ // Create a code owner config that wrongly imports the root folder instead of the '/OWNERS'
+ // file.
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/foo/");
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, /* filePath= */ "/")
+ .build();
+ CodeOwnerConfig codeOwnerConfig =
+ createCodeOwnerConfigWithImport(
+ keyOfImportingCodeOwnerConfig, importType, codeOwnerConfigReference);
+
+ PushOneCommit.Result r =
+ createChange(
+ user,
+ "Add code owners",
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(codeOwnerConfig));
+ assertErrorWithMessages(
+ r,
+ "invalid code owner config files",
+ String.format(
+ "invalid %s import in '%s': '/' is not a code owner config file",
+ importType.getType(),
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getFilePath()));
+ }
+
+ @Test
public void
forMergeCommitsNonResolvableGlobalImportsFromOtherProjectsAreReportedAsWarningsIfImportsDontSpecifyBranch()
throws Exception {
@@ -2215,6 +2270,542 @@
}
@Test
+ public void skipValidationForMergeCommitCreatedViaTheCherryPickRevisionRestApi()
+ throws Exception {
+ // Create a conflicting code owner config file in the target branch.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ // Create another branch.
+ BranchNameKey branchNameKey = BranchNameKey.create(project, "stable");
+ createBranch(branchNameKey);
+
+ // Create a change with a conflicting code owner config file in the other branch.
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ CodeOwnerConfig.Key.create(branchNameKey, Path.of("/"));
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owner config",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder().addCodeOwnerEmail(admin.email()).build())
+ .build()));
+ r.assertOkStatus();
+
+ // Try creating a change that cherry picks the change on the other branch onto master.
+ // The change creation fails because the code owner config file in the other branch
+ // conflicts with the code owner config file in the master branch.
+ CherryPickInput cherryPickInput = new CherryPickInput();
+ cherryPickInput.destination = "master";
+ cherryPickInput.message = "A cherry pick";
+ IntegrationConflictException mergeConflictException =
+ assertThrows(
+ IntegrationConflictException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().cherryPickAsInfo(cherryPickInput));
+ assertThat(mergeConflictException)
+ .hasMessageThat()
+ .contains("Cherry pick failed: merge conflict while merging commits");
+
+ // Try creating the cherry pick change with conflicts. Fails because the code owner config file
+ // contains conflict markers which fails the code owner config file validation.
+ cherryPickInput.allowConflicts = true;
+ ResourceConflictException resourceConflictException =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().cherryPickAsInfo(cherryPickInput));
+ assertThat(resourceConflictException)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "[code-owners] invalid code owner config file '/%s'",
+ getCodeOwnerConfigFileName()));
+
+ // Create the cherry pick change with skipping code owners validation.
+ cherryPickInput.validationOptions =
+ ImmutableMap.of(
+ String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME), "true");
+ gApi.changes().id(r.getChangeId()).current().cherryPickAsInfo(cherryPickInput);
+ }
+
+ @Test
+ public void userWithoutCapabilitySkipValidationCannotSkipValidationWithCherryPickRevisionRestApi()
+ throws Exception {
+ // Create another branch.
+ BranchNameKey branchNameKey = BranchNameKey.create(project, "stable");
+ createBranch(branchNameKey);
+
+ // Create a change with a code owner config file in the other branch.
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ CodeOwnerConfig.Key.create(branchNameKey, Path.of("/"));
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owner config",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder().addCodeOwnerEmail(admin.email()).build())
+ .build()));
+ r.assertOkStatus();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ CherryPickInput cherryPickInput = new CherryPickInput();
+ cherryPickInput.destination = "master";
+ cherryPickInput.message = "A cherry pick";
+ cherryPickInput.validationOptions =
+ ImmutableMap.of(
+ String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME), "true");
+ ResourceConflictException resourceConflictException =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().cherryPickAsInfo(cherryPickInput));
+ assertThat(resourceConflictException)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "[code-owners] %s for plugin code-owners not permitted",
+ SkipCodeOwnerConfigValidationCapability.ID));
+ }
+
+ @Test
+ public void skipValidationForRebaseWithConflicts() throws Exception {
+ // Create code owner config with 'admin' as code owner.
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ // Create a change with a conflicting code owner config that makes 'user' the code owner.
+ // No need to reset the repo, since the commit that created to code owner config above wasn't
+ // fetched into testRepo.
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Add user as code owner",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addCodeOwnerSet(CodeOwnerSet.builder().addCodeOwnerEmail(user.email()).build())
+ .build()));
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.allowConflicts = true;
+ rebaseInput.validationOptions =
+ ImmutableMap.of(
+ String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME), "true");
+ ChangeInfo changeInfo = gApi.changes().id(r.getChangeId()).current().rebaseAsInfo(rebaseInput);
+ assertThat(changeInfo.containsGitConflicts).isTrue();
+ }
+
+ @Test
+ public void userWithoutCapabilitySkipValidationCannotSkipValidationWithRebase() throws Exception {
+ // Create code owner config with 'admin' as code owner.
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ // Create a change with a conflicting code owner config that makes 'user' the code owner.
+ // No need to reset the repo, since the commit that created to code owner config above wasn't
+ // fetched into testRepo.
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Add user as code owner",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addCodeOwnerSet(CodeOwnerSet.builder().addCodeOwnerEmail(user.email()).build())
+ .build()));
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ requestScopeOperations.setApiUser(user.id());
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.REBASE).ref(RefNames.REFS_HEADS + "*").group(REGISTERED_USERS))
+ .update();
+
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.allowConflicts = true;
+ rebaseInput.validationOptions =
+ ImmutableMap.of(
+ String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME), "true");
+ ResourceConflictException resourceConflictException =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().rebaseAsInfo(rebaseInput));
+ assertThat(resourceConflictException)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "[code-owners] %s for plugin code-owners not permitted",
+ SkipCodeOwnerConfigValidationCapability.ID));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "true")
+ public void cannotCreateBranchWithInvalidCodeOwnerConfigFileViaRestApi() throws Exception {
+ // Add a non code owner config file to verify that it is not validated as code owner config file
+ PushOneCommit.Result r = pushFactory.create(admin.newIdent(), testRepo).to("refs/heads/master");
+ r.assertOkStatus();
+
+ // Create code owner configs with a non-existing user as code owner.
+ // We create 2 code owner configs with different commits so that it's tested that the validator
+ // checks all code owner config files and not only those added in the last commit.
+ String unknownEmail = "non-existing@example.com";
+ CodeOwnerConfig.Key codeOwnerConfigKey1 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+ CodeOwnerConfig.Key codeOwnerConfigKey2 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+
+ BranchInput input = new BranchInput();
+ input.ref = "new";
+ input.revision = projectOperations.project(project).getHead("master").name();
+
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.projects().name(project.get()).branch(input.ref).create(input));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Validation for creation of ref 'refs/heads/new' in project %s failed:\n"
+ + "[code-owners] invalid code owner config files:\n"
+ + " ERROR: code owner email '%s' in '%s' cannot be resolved for %s\n"
+ + " ERROR: code owner email '%s' in '%s' cannot be resolved for %s",
+ project,
+ unknownEmail,
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey1).getFilePath(),
+ identifiedUserFactory.create(admin.id()).getLoggableName(),
+ unknownEmail,
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getFilePath(),
+ identifiedUserFactory.create(admin.id()).getLoggableName()));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "true")
+ public void skipValidationForBranchCreationViaRestApi() throws Exception {
+ // Create code owner config with a non-existing user as code owner.
+ String unknownEmail = "non-existing@example.com";
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+
+ BranchInput input = new BranchInput();
+ input.ref = "new";
+ input.revision = projectOperations.project(project).getHead("master").name();
+
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.projects().name(project.get()).branch(input.ref).create(input));
+ assertThat(exception)
+ .hasMessageThat()
+ .contains("[code-owners] invalid code owner config files:");
+
+ input.validationOptions =
+ ImmutableMap.of(
+ String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME), "true");
+ gApi.projects().name(project.get()).branch(input.ref).create(input);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "true")
+ public void userWithoutCapabilitySkipValidationCannotSkipValidationForBranchCreationViaRestApi()
+ throws Exception {
+ // Create code owner config with a non-existing user as code owner.
+ String unknownEmail = "non-existing@example.com";
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ BranchInput input = new BranchInput();
+ input.ref = "new";
+ input.revision = projectOperations.project(project).getHead("master").name();
+ input.validationOptions =
+ ImmutableMap.of(
+ String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME), "true");
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.CREATE).ref(RefNames.REFS_HEADS + "*").group(REGISTERED_USERS))
+ .update();
+
+ ResourceConflictException resourceConflictException =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.projects().name(project.get()).branch(input.ref).create(input));
+ assertThat(resourceConflictException)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "Validation for creation of ref 'refs/heads/new' in project %s failed:\n"
+ + "[code-owners] skipping code owner config validation not allowed:\n"
+ + " ERROR: %s for plugin code-owners not permitted",
+ project, SkipCodeOwnerConfigValidationCapability.ID));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "true")
+ public void cannotCreateBranchWithInvalidCodeOwnerConfigFileViaPush() throws Exception {
+ // Add a non code owner config file to verify that it is not validated as code owner config file
+ PushOneCommit.Result r = pushFactory.create(admin.newIdent(), testRepo).to("refs/heads/master");
+ r.assertOkStatus();
+
+ // Create code owner configs with a non-existing user as code owner.
+ // We create 2 code owner configs with different commits so that it's tested that the validator
+ // checks all code owner config files and not only those added in the last commit.
+ String unknownEmail = "non-existing@example.com";
+ CodeOwnerConfig.Key codeOwnerConfigKey1 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+ CodeOwnerConfig.Key codeOwnerConfigKey2 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+
+ RevCommit head = projectOperations.project(project).getHead("master");
+ testRepo.git().fetch().call();
+ testRepo.reset(head.name());
+
+ PushResult r2 =
+ pushHead(
+ testRepo,
+ "refs/heads/new",
+ /* pushTags= */ false,
+ /* force= */ false,
+ /* pushOptions= */ ImmutableList.of());
+ assertPushRejected(
+ r2,
+ "refs/heads/new",
+ String.format(
+ "Validation for creation of ref 'refs/heads/new' in project %s failed:\n"
+ + "[code-owners] invalid code owner config files:\n"
+ + " ERROR: code owner email '%s' in '%s' cannot be resolved for %s\n"
+ + " ERROR: code owner email '%s' in '%s' cannot be resolved for %s",
+ project,
+ unknownEmail,
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey1).getFilePath(),
+ identifiedUserFactory.create(admin.id()).getLoggableName(),
+ unknownEmail,
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getFilePath(),
+ identifiedUserFactory.create(admin.id()).getLoggableName()));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "true")
+ public void skipValidationForBranchCreationViaPush() throws Exception {
+ // Create code owner config with a non-existing user as code owner.
+ String unknownEmail = "non-existing@example.com";
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+
+ RevCommit head = projectOperations.project(project).getHead("master");
+ testRepo.git().fetch().call();
+ testRepo.reset(head.name());
+
+ PushResult r =
+ pushHead(
+ testRepo,
+ "refs/heads/new",
+ /* pushTags= */ false,
+ /* force= */ false,
+ /* pushOptions= */ ImmutableList.of());
+ assertPushRejected(
+ r,
+ "refs/heads/new",
+ String.format(
+ "Validation for creation of ref 'refs/heads/new' in project %s failed:\n"
+ + "[code-owners] invalid code owner config files:\n"
+ + " ERROR: code owner email '%s' in '%s' cannot be resolved for %s",
+ project,
+ unknownEmail,
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ identifiedUserFactory.create(admin.id()).getLoggableName()));
+
+ r =
+ pushHead(
+ testRepo,
+ "refs/heads/new",
+ /* pushTags= */ false,
+ /* force= */ false,
+ /* pushOptions= */ ImmutableList.of(
+ String.format(
+ "code-owners~%s=true", SkipCodeOwnerConfigValidationPushOption.NAME)));
+ assertPushOk(r, "refs/heads/new");
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "true")
+ public void userWithoutCapabilitySkipValidationCannotSkipValidationForBranchCreationViaPush()
+ throws Exception {
+ // Create code owner config with a non-existing user as code owner.
+ String unknownEmail = "non-existing@example.com";
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.CREATE).ref(RefNames.REFS_HEADS + "*").group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+ TestRepository<InMemoryRepository> userRepo = cloneProject(project, user);
+ PushResult r =
+ pushHead(
+ userRepo,
+ "refs/heads/new",
+ /* pushTags= */ false,
+ /* force= */ false,
+ /* pushOptions= */ ImmutableList.of(
+ String.format(
+ "code-owners~%s=true", SkipCodeOwnerConfigValidationPushOption.NAME)));
+ assertPushRejected(
+ r,
+ "refs/heads/new",
+ String.format(
+ "Validation for creation of ref 'refs/heads/new' in project %s failed:\n"
+ + "[code-owners] skipping code owner config validation not allowed:\n"
+ + " ERROR: %s for plugin code-owners not permitted",
+ project, SkipCodeOwnerConfigValidationCapability.ID));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "false")
+ public void onBranchCreationValidationDisabled() throws Exception {
+ // Create a code owner config with a non-existing user as code owner.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("non-existing@example.com")
+ .create();
+
+ RevCommit head = projectOperations.project(project).getHead("master");
+ testRepo.git().fetch().call();
+ testRepo.reset(head.name());
+
+ PushResult r =
+ pushHead(
+ testRepo,
+ "refs/heads/new",
+ /* pushTags= */ false,
+ /* force= */ false,
+ /* pushOptions= */ ImmutableList.of());
+ assertPushOk(r, "refs/heads/new");
+ assertThat(r.getMessages())
+ .contains(
+ "hint: [code-owners] skipping validation of code owner config files\n"
+ + "hint: [code-owners] code owners config validation is disabled");
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "dry_run")
+ public void canCreateBranchWithInvalidCodeOwnerConfigIfValidationIsDoneAsDryRun()
+ throws Exception {
+ // Create a code owner config with a non-existing user as code owner.
+ String unknownEmail = "non-existing@example.com";
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(unknownEmail)
+ .create();
+
+ RevCommit head = projectOperations.project(project).getHead("master");
+ testRepo.git().fetch().call();
+ testRepo.reset(head.name());
+
+ PushResult r =
+ pushHead(
+ testRepo,
+ "refs/heads/new",
+ /* pushTags= */ false,
+ /* force= */ false,
+ /* pushOptions= */ ImmutableList.of());
+ assertPushOk(r, "refs/heads/new");
+ assertThat(r.getMessages())
+ .contains(
+ String.format(
+ "ERROR: [code-owners] invalid code owner config files\n"
+ + "ERROR: [code-owners] code owner email '%s' in '%s' cannot be resolved for"
+ + " %s",
+ unknownEmail,
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ identifiedUserFactory.create(admin.id()).getLoggableName()));
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
@GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "true")
public void canUploadAndSubmitConfigWithUnresolvableCodeOwners() throws Exception {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerHasOperandsIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerHasOperandsIT.java
index 070cbb5..f8b21aa 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerHasOperandsIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerHasOperandsIT.java
@@ -44,7 +44,6 @@
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerApprovalHasOperand;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
-import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.inject.Inject;
@@ -97,9 +96,6 @@
}
@Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void hasApproval_satisfied() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
@@ -123,9 +119,6 @@
}
@Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void hasApproval_unsatisfiedIfChangeIsClosed() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
@@ -151,15 +144,11 @@
gApi.changes().id(changeId).current().submit();
changeInfo = gApi.changes().id(changeId).get(ListChangesOption.SUBMIT_REQUIREMENTS);
// When the change is merged, submit requirement results are persisted in NoteDb. Later lookups
- // return the persisted snapshot. Currently writing to NoteDb is disabled.
- // TODO(ghareeb): update this check when we enable writing to NoteDb again.
- assertNonExistentSubmitRequirement(changeInfo.submitRequirements, "Code-Owner-Approval");
+ // return the persisted snapshot.
+ assertSubmitRequirement(changeInfo.submitRequirements, "Code-Owner-Approval", SATISFIED);
}
@Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void hasApproval_internalServerError() throws Exception {
ChangeData changeData = createChange().getChange();
@@ -186,9 +175,6 @@
}
@Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void hasApproval_ruleErrorForNonParsableCodeOwnerConfig() throws Exception {
String nameOfInvalidCodeOwnerConfigFile = getCodeOwnerConfigFileName();
createNonParseableCodeOwnerConfig(nameOfInvalidCodeOwnerConfigFile);
@@ -205,9 +191,6 @@
@Test
@GerritConfig(name = "plugin.code-owners.disabled", value = "true")
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void hasEnabled_notMatchingWhenCodeOwnersIsDisabledForTheChange() throws Exception {
Change change =
createChange("Change Adding A File", "foo/bar.baz", "file content").getChange().change();
@@ -258,15 +241,6 @@
.collect(toImmutableList())));
}
- private void assertNonExistentSubmitRequirement(
- Collection<SubmitRequirementResultInfo> requirements, String name) {
- for (SubmitRequirementResultInfo requirement : requirements) {
- if (requirement.name.equals(name)) {
- throw new AssertionError("Found a submit requirement with name " + name);
- }
- }
- }
-
private String format(String query, Iterable<Change.Id> actualIds, Change.Id... expectedChanges)
throws RestApiException {
return "query '"
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index fef1205..f3e7d1b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -16,8 +16,10 @@
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.allowLabel;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerStatusInfoSubject.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.LegacySubmitRequirementInfoSubject.assertThatCollection;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
@@ -25,8 +27,10 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestMetricMaker;
+import com.google.gerrit.acceptance.TestProjectInput;
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.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.RefNames;
@@ -53,6 +57,7 @@
/** Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.CodeOwnerSubmitRule}. */
public class CodeOwnerSubmitRuleIT extends AbstractCodeOwnersIT {
+ @Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
@Inject private TestMetricMaker testMetricMaker;
@@ -100,7 +105,17 @@
}
@Test
- public void changeWithInsufficentReviewersIsNotSubmittable() throws Exception {
+ public void changeWithInsufficientReviewersIsNotSubmittable() throws Exception {
+ testChangeWithInsufficientReviewersIsNotSubmittable();
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithInsufficientReviewersIsNotSubmittable() throws Exception {
+ testChangeWithInsufficientReviewersIsNotSubmittable();
+ }
+
+ private void testChangeWithInsufficientReviewersIsNotSubmittable() throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Approve by a non-code-owner.
@@ -125,7 +140,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("NOT_READY");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Try to submit the change.
@@ -137,7 +152,7 @@
.isEqualTo(
String.format(
"Failed to submit 1 change due to the following problems:\n"
- + "Change %d: Submit requirement not fulfilled: Code Owners",
+ + "Change %d: submit requirement 'Code-Owners' is unsatisfied.",
changeInfo._number));
}
@@ -151,10 +166,23 @@
.addCodeOwnerEmail(user.email())
.create();
+ testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(user);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithPendingCodeOwnerApprovalsIsNotSubmittable() throws Exception {
+ setAsDefaultCodeOwners(user);
+
+ testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(user);
+ }
+
+ private void testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(TestAccount codeOwner)
+ throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Add a reviewer that is a code owner.
- gApi.changes().id(changeId).addReviewer(user.email());
+ gApi.changes().id(changeId).addReviewer(codeOwner.email());
// Approve by a non-code-owner.
approve(changeId);
@@ -178,7 +206,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("NOT_READY");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Try to submit the change.
@@ -190,7 +218,7 @@
.isEqualTo(
String.format(
"Failed to submit 1 change due to the following problems:\n"
- + "Change %d: Submit requirement not fulfilled: Code Owners",
+ + "Change %d: submit requirement 'Code-Owners' is unsatisfied.",
changeInfo._number));
}
@@ -201,13 +229,33 @@
.project(project)
.branch("master")
.folderPath("/foo/")
- .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
.create();
+ testChangeWithCodeOwnerApprovalsIsSubmittable(user);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithCodeOwnerApprovalsIsSubmittable() throws Exception {
+ setAsDefaultCodeOwners(user);
+
+ testChangeWithCodeOwnerApprovalsIsSubmittable(user);
+ }
+
+ private void testChangeWithCodeOwnerApprovalsIsSubmittable(TestAccount codeOwner)
+ throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Approve by a code-owner.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(codeOwner.id());
approve(changeId);
+ requestScopeOperations.setApiUser(admin.id());
// Verify that the code owner status for the changed file is APPROVED.
CodeOwnerStatusInfo codeOwnerStatus =
@@ -228,7 +276,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Submit the change.
@@ -239,6 +287,17 @@
@Test
@GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
public void changeWithOverrideApprovalIsSubmittable() throws Exception {
+ testChangeWithOverrideApprovalIsSubmittable();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithOverrideApprovalIsSubmittable() throws Exception {
+ testChangeWithOverrideApprovalIsSubmittable();
+ }
+
+ private void testChangeWithOverrideApprovalIsSubmittable() throws Exception {
createOwnersOverrideLabel();
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
@@ -268,7 +327,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(gApi.changes().id(changeId).get().requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Submit the change.
@@ -305,7 +364,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("NOT_READY");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Try to submit the change.
@@ -348,7 +407,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Submit the change.
@@ -404,7 +463,9 @@
.isEqualTo(
String.format(
"Failed to submit 1 change due to the following problems:\n"
- + "Change %s: submit rule error: Failed to evaluate code owner statuses for"
+ + "Change %s: submit requirement 'Code-Owners' has an"
+ + " error: Submittability expression result has an error:"
+ + " Failed to evaluate code owner statuses for"
+ " patch set 1 of change %s (cause: invalid code owner config file '%s'"
+ " (project = %s, branch = master):\n %s).%s",
changeInfo._number,
@@ -456,7 +517,9 @@
.isEqualTo(
String.format(
"Failed to submit 1 change due to the following problems:\n"
- + "Change %s: submit rule error: Failed to evaluate code owner statuses for"
+ + "Change %s: submit requirement 'Code-Owners' has an"
+ + " error: Submittability expression result has an error:"
+ + " Failed to evaluate code owner statuses for"
+ " patch set 1 of change %s (cause: invalid code owner config file '%s'"
+ " (project = %s, branch = master):\n %s).",
changeInfo._number,
@@ -482,7 +545,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Submit the change.
@@ -526,7 +589,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Submit the change.
@@ -610,7 +673,7 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
// Submit the change.
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
index bfc0283..70fb164 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
@@ -83,6 +83,18 @@
}
@Test
+ public void noChangeMessageAddedIfInvalidCodeOwnerConfigFilesExist() throws Exception {
+ createNonParseableCodeOwnerConfig(getCodeOwnerConfigFileName());
+
+ String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
+
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Uploaded patch set 1.");
+ }
+
+ @Test
public void changeMessageListsOwnedPaths() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
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 f7f94be..b2a6630 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -803,6 +803,63 @@
ObjectIds.abbreviateName(r.getCommit(), testRepo.getRevWalk().getObjectReader())));
}
+ @Test
+ public void warnIfCodeOwnersConfigurationIsDoneInProjectConfig_configWithPluginCodeOwnersSection()
+ throws Exception {
+ fetchRefsMetaConfig();
+ Config cfg = new Config();
+ cfg.setEnum(
+ /* section= */ "plugin",
+ /* subsection= */ "code-owners",
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ FallbackCodeOwners.ALL_USERS);
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change", "project.config", cfg.toText());
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ r.assertMessage(
+ String.format(
+ "hint: commit %s: Section 'plugin.code-owners' in project.config is ignored and has no"
+ + " effect. The configuration for the code-owners plugin must be done in"
+ + " code-owners.config.",
+ ObjectIds.abbreviateName(r.getCommit(), testRepo.getRevWalk().getObjectReader())));
+ }
+
+ @Test
+ public void warnIfCodeOwnersConfigurationIsDoneInProjectConfig_configWithCodeOwnersSection()
+ throws Exception {
+ fetchRefsMetaConfig();
+ Config cfg = new Config();
+ cfg.setEnum(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ FallbackCodeOwners.ALL_USERS);
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change", "project.config", cfg.toText());
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ r.assertMessage(
+ String.format(
+ "hint: commit %s: Section 'codeOwners' in project.config is ignored and has no effect."
+ + " The configuration for the code-owners plugin must be done in code-owners.config.",
+ ObjectIds.abbreviateName(r.getCommit(), testRepo.getRevWalk().getObjectReader())));
+ }
+
+ @Test
+ public void noWarningIfProjectConfigIsUpdatedWithoutCodeOwnerSettings() throws Exception {
+ fetchRefsMetaConfig();
+ Config cfg = new Config();
+ cfg.setString(
+ /* section= */ "foo", /* subsection= */ null, /* name= */ "bar", /* value= */ "baz");
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change", "project.config", cfg.toText());
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ r.assertNotMessage(
+ "The configuration for the code-owners plugin must be done in code-owners.config.");
+ }
+
private void fetchRefsMetaConfig() throws Exception {
fetch(testRepo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
testRepo.reset(RefNames.REFS_CONFIG);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigForPathInBranchIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigForPathInBranchIT.java
index 3048707..5ddd85f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigForPathInBranchIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigForPathInBranchIT.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerConfigInfoSubject.assertThatOptional;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
@@ -25,6 +26,8 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import java.util.Optional;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
/**
@@ -39,6 +42,28 @@
public class GetCodeOwnerConfigForPathInBranchIT extends AbstractCodeOwnersIT {
@Test
@GerritConfig(name = "plugin.code-owners.enableExperimentalRestEndpoints", value = "true")
+ public void getCodeOwnerConfigForNonExistingBranch() throws Exception {
+ RestResponse response =
+ adminRestSession.get(
+ String.format(
+ "/projects/%s/branches/non-existing/code_owners.config/path", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableExperimentalRestEndpoints", value = "true")
+ public void getCodeOwnerConfigFromSymbolicRefPointingToAnUnbornBranch() throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ repo.updateRef(Constants.HEAD, true).link("refs/heads/non-existing");
+ }
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/HEAD/code_owners.config/path", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableExperimentalRestEndpoints", value = "true")
public void getNonExistingCodeOwnerConfig() throws Exception {
assertThatOptional(codeOwnerConfigsApiFactory.branch(project, "master").get("/")).isEmpty();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
index dfd97d4..a78d420 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
@@ -21,6 +21,7 @@
import static java.util.stream.Collectors.toList;
import static org.junit.Assert.fail;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
@@ -38,6 +39,8 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSetModification;
import com.google.inject.Inject;
import java.util.List;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Test;
@@ -60,6 +63,25 @@
}
@Test
+ public void getCodeOwnersForNonExistingBranch() throws Exception {
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/non-existing/code_owners/path", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
+ public void getCodeOwnersFromSymbolicRefPointingToAnUnbornBranch() throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ repo.updateRef(Constants.HEAD, true).link("refs/heads/non-existing");
+ }
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/HEAD/code_owners/path", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
public void getCodeOwnersOrderNotDefinedIfCodeOwnersHaveTheSameScoring() throws Exception {
TestAccount user2 = accountCreator.user2();
TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index efd2166..d3fe19a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -65,6 +65,19 @@
}
@Test
+ public void changeMessageNotExtendedIfInvalidCodeOwnerConfigFilesExist() throws Exception {
+ createNonParseableCodeOwnerConfig(getCodeOwnerConfigFileName());
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ recommend(changeId);
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Code-Review+1");
+ }
+
+ @Test
public void changeMessageListsNewlyApprovedPaths() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java
index 5834499..e632f2a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java
@@ -493,6 +493,32 @@
}
@Test
+ public void setEnableValidationOnBranchCreation() throws Exception {
+ assertThat(
+ codeOwnersPluginConfiguration
+ .getProjectConfig(project)
+ .getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+
+ CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput();
+ input.enableValidationOnBranchCreation = CodeOwnerConfigValidationPolicy.TRUE;
+ projectCodeOwnersApiFactory.project(project).updateConfig(input);
+ assertThat(
+ codeOwnersPluginConfiguration
+ .getProjectConfig(project)
+ .getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE);
+
+ input.enableValidationOnBranchCreation = CodeOwnerConfigValidationPolicy.FALSE;
+ projectCodeOwnersApiFactory.project(project).updateConfig(input);
+ assertThat(
+ codeOwnersPluginConfiguration
+ .getProjectConfig(project)
+ .getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
public void setEnableValidationOnCommitReceived() throws Exception {
assertThat(
codeOwnersPluginConfiguration
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/batch/BUILD b/javatests/com/google/gerrit/plugins/codeowners/acceptance/batch/BUILD
index cef4da8..1d3e5a0 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/batch/BUILD
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/batch/BUILD
@@ -12,6 +12,8 @@
group = "acceptance_batch",
deps = [
"//plugins/code-owners:code-owners__plugin",
+ "//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/acceptance",
+ "//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/testing",
],
)
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/batch/CodeOwnerSubmitRuleBatchIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/batch/CodeOwnerSubmitRuleBatchIT.java
index 4235792..17d481c 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/batch/CodeOwnerSubmitRuleBatchIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/batch/CodeOwnerSubmitRuleBatchIT.java
@@ -1,3 +1,17 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
package com.google.gerrit.plugins.codeowners.acceptance.batch;
import static com.google.common.truth.Truth.assertThat;
@@ -8,15 +22,16 @@
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestPlugin;
-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.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.testing.LegacySubmitRequirementInfoSubject;
import com.google.inject.Inject;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
+import org.junit.Before;
import org.junit.Test;
/**
@@ -26,14 +41,29 @@
*/
@TestPlugin(
name = "code-owners",
- sysModule = "com.google.gerrit.plugins.codeowners.module.BatchModule")
+ sysModule = "com.google.gerrit.plugins.codeowners.acceptance.TestBatchModule")
public class CodeOwnerSubmitRuleBatchIT extends LightweightPluginDaemonTest {
@Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
+ private CodeOwnerConfigOperations codeOwnerConfigOperations;
+
+ @Before
+ public void testSetup() throws Exception {
+ codeOwnerConfigOperations =
+ plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class);
+ }
+
@Test
- @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
public void invokeCodeOwnerSubmitRule() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
// Upload a change as a non-code owner.
TestRepository<InMemoryRepository> testRepo = cloneProject(project, user);
PushOneCommit push =
@@ -57,11 +87,10 @@
LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("NOT_READY");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
- // Approve by a project owner who is code owner since project owners are configured as fallback
- // code owners.
+ // Approve by a code owner.
requestScopeOperations.setApiUser(admin.id());
approve(changeId);
@@ -72,7 +101,7 @@
// Check the submit requirement.
submitRequirementInfoSubject = assertThatCollection(changeInfo.requirements).onlyElement();
submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
- submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java
index 1220c6c..506d204 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java
@@ -212,81 +212,6 @@
.isEqualTo(CodeOwnerStatus.APPROVED);
}
- @Test
- @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
- public void notApprovedByUser_projectOwnersAreFallbackCodeOwner() throws Exception {
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- ChangeNotes changeNotes = getChangeNotes(changeId);
-
- // Verify that the file would not be approved by the user since the user is not a project owner.
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesForAccount(
- changeNotes, changeNotes.getCurrentPatchSet(), user.id());
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
- public void approvedByProjectOwner_projectOwnersAreFallbackCodeOwner() throws Exception {
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- ChangeNotes changeNotes = getChangeNotes(changeId);
-
- // Verify that the file would be approved by the 'admin' user since the 'admin' user is a
- // project owner.
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesForAccount(
- changeNotes, changeNotes.getCurrentPatchSet(), admin.id());
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
- public void notApprovedByProjectOwner_projectOwnersAreFallbackCodeOwner_otherOwnerDefined()
- throws Exception {
- TestAccount codeOwner =
- accountCreator.create(
- "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
- setAsRootCodeOwners(codeOwner);
-
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- ChangeNotes changeNotes = getChangeNotes(changeId);
-
- // Verify that the file would be not approved by the 'admin' user. The 'admin' user is a
- // project owner, but fallback code owners are not applied if code ownership was explicitly
- // defined.
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesForAccount(
- changeNotes, changeNotes.getCurrentPatchSet(), admin.id());
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
@GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
@Test
public void approvedByFallbackCodeOwner() throws Exception {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 34642d5..915a108 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -42,7 +42,6 @@
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelDefinitionInput;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
@@ -1482,19 +1481,204 @@
}
@Test
- public void getStatus_branchDeleted() throws Exception {
+ public void getStatus_branchDeleted_defaultCodeOwner() throws Exception {
String branchName = "tempBranch";
createBranch(BranchNameKey.create(project, branchName));
- String changeId = createChange("refs/for/" + branchName).getChangeId();
+ // Create a change as a 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();
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = ImmutableList.of(branchName);
gApi.projects().name(project.get()).deleteBranches(input);
- ResourceConflictException exception =
- assertThrows(ResourceConflictException.class, () -> getFileCodeOwnerStatuses(changeId));
- assertThat(exception).hasMessageThat().isEqualTo("destination branch not found");
+ testGetStatusBranchDoesNotExistWithDefaultCodeOwner(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void getStatus_initialChange_defaultCodeOwner() throws Exception {
+ // Create a change as a 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();
+
+ testGetStatusBranchDoesNotExistWithDefaultCodeOwner(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithDefaultCodeOwner(String changeId, Path path)
+ throws Exception {
+ setAsDefaultCodeOwners(admin);
+
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add default code owner as a reviewer.
+ gApi.changes().id(changeId).addReviewer(admin.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a default code owner",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+
+ // Approve as default code owner.
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a default code owner",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ public void getStatus_branchDeleted_globalCodeOwner() throws Exception {
+ // Create a bot user that is a global code owner.
+ TestAccount bot =
+ accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExistWithGlobalCodeOwner(changeId, path, bot);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ public void getStatus_initialChange_globalCodeOwner() throws Exception {
+ // Create a bot user that is a global code owner.
+ TestAccount bot =
+ accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExistWithGlobalCodeOwner(changeId, path, bot);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithGlobalCodeOwner(
+ String changeId, Path path, TestAccount globalCodeOwner) throws Exception {
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add global code owner as a reviewer.
+ gApi.changes().id(changeId).addReviewer(globalCodeOwner.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a global code owner",
+ AccountTemplateUtil.getAccountTemplate(globalCodeOwner.id()))));
+
+ // Approve as default code owner.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(globalCodeOwner.id());
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a global code owner",
+ AccountTemplateUtil.getAccountTemplate(globalCodeOwner.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void getStatus_branchDeleted_override() throws Exception {
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExistWithOverride(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void getStatus_initialChange_override() throws Exception {
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExistWithOverride(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithOverride(String changeId, Path path)
+ throws Exception {
+ createOwnersOverrideLabel();
+
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Apply an override
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
}
@Test
@@ -1571,7 +1755,7 @@
// make the override label sticky
LabelDefinitionInput input = new LabelDefinitionInput();
- input.copyAnyScore = true;
+ input.copyCondition = "is:ANY";
gApi.projects().name(project.get()).label("Owners-Override").update(input);
// Create a change as a user that is not a code owner.
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
index c91d897..5c31eed 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
@@ -16,11 +16,15 @@
import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatCollection;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
@@ -459,6 +463,70 @@
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
+ @Test
+ public void getStatus_branchDeleted() throws Exception {
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a 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();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExist(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void getStatus_initialChange() throws Exception {
+ // Create a change as a 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();
+
+ testGetStatusBranchDoesNotExist(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExist(String changeId, Path path) throws Exception {
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add code owner as a reviewer (all users are fallback code owners).
+ gApi.changes().id(changeId).addReviewer(admin.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a fallback code owner (all users are fallback code owners)",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+
+ // Approve as a code owner (all users are fallback code owners).
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a fallback code owner"
+ + " (all users are fallback code owners)",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+ }
+
private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
throws Exception {
return codeOwnerApprovalCheck.getFileStatusesAsSet(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
deleted file mode 100644
index dfb0144..0000000
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
+++ /dev/null
@@ -1,692 +0,0 @@
-// Copyright (C) 2021 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.plugins.codeowners.backend;
-
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatCollection;
-import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
-
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-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.request.RequestScopeOperations;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.extensions.api.changes.ReviewInput;
-import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
-import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
-import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
-import com.google.gerrit.plugins.codeowners.util.JgitPath;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.util.AccountTemplateUtil;
-import com.google.gerrit.testing.ConfigSuite;
-import com.google.inject.Inject;
-import java.nio.file.Path;
-import java.nio.file.Paths;
-import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
-import org.eclipse.jgit.junit.TestRepository;
-import org.eclipse.jgit.lib.Config;
-import org.junit.Before;
-import org.junit.Test;
-
-/** Tests for {@link CodeOwnerApprovalCheck} with PROJECT_OWNERS as fallback code owners. */
-public class CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest
- extends AbstractCodeOwnersTest {
- @Inject private ChangeNotes.Factory changeNotesFactory;
- @Inject private RequestScopeOperations requestScopeOperations;
- @Inject private ProjectOperations projectOperations;
-
- private CodeOwnerApprovalCheck codeOwnerApprovalCheck;
-
- /** Returns a {@code gerrit.config} that configures all users as fallback code owners. */
- @ConfigSuite.Default
- public static Config defaultConfig() {
- Config cfg = new Config();
- cfg.setEnum(
- "plugin",
- "code-owners",
- GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
- FallbackCodeOwners.PROJECT_OWNERS);
- return cfg;
- }
-
- @Before
- public void setUpCodeOwnersPlugin() throws Exception {
- codeOwnerApprovalCheck = plugin.getSysInjector().getInstance(CodeOwnerApprovalCheck.class);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
- public void approvedByGlobalCodeOwner() throws Exception {
- // Create a bot user that is a global code owner.
- TestAccount bot =
- accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
-
- // Create a change as a 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.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Let the bot approve the change.
- projectOperations
- .project(project)
- .forUpdate()
- .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
- .update();
- requestScopeOperations.setApiUser(bot.id());
- approve(changeId);
-
- // Check that the file is approved now.
- requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a global code owner",
- AccountTemplateUtil.getAccountTemplate(bot.id()))));
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", values = "bot@example.com")
- public void globalCodeOwner_noImplicitApproval() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", values = "bot@example.com")
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void globalCodeOwner_noImplicitApproval_uploaderDoesntMatchChangeOwner() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", values = "bot@example.com")
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void globalCodeOwner_withImplicitApproval() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
- }
-
- private void testImplicitlyApprovedByGlobalCodeOwner(
- boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
- TestAccount bot =
- accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
- TestAccount projectOwner = admin;
-
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange(bot, "Change Adding A File", JgitPath.of(path).get(), "file content")
- .getChangeId();
-
- if (uploaderMatchesChangeOwner) {
- amendChange(bot, changeId);
- } else {
- amendChange(projectOwner, changeId);
- }
-
- FileCodeOwnerStatus expectedFileCodeOwnerStatus;
- if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.APPROVED,
- String.format(
- "implicitly approved by the patch set uploader %s who is a global code"
- + " owner",
- AccountTemplateUtil.getAccountTemplate(bot.id())));
- } else {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses).containsExactly(expectedFileCodeOwnerStatus);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
- public void globalCodeOwnerAsReviewer() throws Exception {
- // Create a bot user that is a global code owner.
- TestAccount bot =
- accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
-
- // Create a change as a 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 status of the file is INSUFFICIENT_REVIEWERS.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Add the bot approve as reviewer.
- gApi.changes().id(changeId).addReviewer(bot.email());
-
- // Check that the status of the file is PENDING now.
- requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a global code owner",
- AccountTemplateUtil.getAccountTemplate(bot.id()))));
-
- // Let the bot approve the change.
- projectOperations
- .project(project)
- .forUpdate()
- .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
- .update();
- requestScopeOperations.setApiUser(bot.id());
- approve(changeId);
-
- // Check that the file is approved now.
- requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a global code owner",
- AccountTemplateUtil.getAccountTemplate(bot.id()))));
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
- public void approvedByAnyoneWhenEveryoneIsGlobalCodeOwner() throws Exception {
- // Create a change.
- 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 (the change owner is a global code owner, but
- // implicit approvals are disabled).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Add an approval by a user that is a code owner only through the global code ownership.
- approve(changeId);
-
- // Check that the file is approved now.
- requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a global code owner"
- + " (all users are global code owners)",
- AccountTemplateUtil.getAccountTemplate(admin.id()))));
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
- public void everyoneIsGlobalCodeOwner_noImplicitApproval() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void everyoneIsGlobalCodeOwner_noImplicitApproval_uploaderDoesntMatchChangeOwner()
- throws Exception {
- testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void everyoneIsGlobalCodeOwner_withImplicitApproval() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
- }
-
- private void testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
- TestAccount projectOwner = admin;
- TestAccount otherProjectOwner = accountCreator.admin2();
-
- // Create a change as a user that is a project code owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
-
- if (uploaderMatchesChangeOwner) {
- amendChange(projectOwner, changeId);
- } else {
- amendChange(otherProjectOwner, changeId);
- }
-
- FileCodeOwnerStatus expectedFileCodeOwnerStatus;
- if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.APPROVED,
- String.format(
- "implicitly approved by the patch set uploader %s who is a global code owner"
- + " (all users are global code owners)",
- AccountTemplateUtil.getAccountTemplate(projectOwner.id())));
- } else {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses).containsExactly(expectedFileCodeOwnerStatus);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
- public void anyReviewerWhenEveryoneIsGlobalCodeOwner() throws Exception {
- // Create a change as a user that is a code owner only through the global code ownership.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
-
- // Verify that the status of the file is INSUFFICIENT_REVIEWERS (since there is no implicit
- // approval by default).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Add a user as reviewer that is a code owner only through the global code ownership.
- gApi.changes().id(changeId).addReviewer(user.email());
-
- // Check that the status of the file is PENDING now.
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a global code owner (all users are global code owners)",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
- }
-
- @Test
- public void getStatus_insufficientReviewers() throws Exception {
- TestAccount user2 = accountCreator.user2();
- TestAccount user3 =
- accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
-
- // Create change with a user that is not a project owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
- .getChangeId();
-
- // Add a reviewer that is not a project owner.
- gApi.changes().id(changeId).addReviewer(user2.email());
-
- // Add a Code-Review+1 (= code owner approval) from a user that is not a project owner.
- requestScopeOperations.setApiUser(user3.id());
- recommend(changeId);
-
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
- }
-
- @Test
- public void getStatus_pending() throws Exception {
- TestAccount user2 = accountCreator.user2();
-
- // Create change with a user that is not a project owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
- .getChangeId();
-
- // Add a reviewer that is a project owner.
- gApi.changes().id(changeId).addReviewer(admin.email());
-
- // Add a Code-Review+1 (= code owner approval) from a user that is not a project owner.
- requestScopeOperations.setApiUser(user2.id());
- recommend(changeId);
-
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a fallback code owner"
- + " (all project owners are fallback code owners)",
- AccountTemplateUtil.getAccountTemplate(admin.id()))));
- }
-
- @Test
- public void getStatus_approved() throws Exception {
- // Create change with a user that is not a project owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
- .getChangeId();
-
- // Add a Code-Review+1 from a project owner (by default this counts as code owner approval).
- requestScopeOperations.setApiUser(admin.id());
- recommend(changeId);
-
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a fallback code owner"
- + " (all project owners are fallback code owners)",
- AccountTemplateUtil.getAccountTemplate(admin.id()))));
- }
-
- @Test
- public void getStatus_noImplicitApproval() throws Exception {
- testImplicitApprovalOnGetStatus(
- /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatus_noImplicitApproval_uploaderDoesntMatchChangeOwner() throws Exception {
- testImplicitApprovalOnGetStatus(
- /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatus_withImplicitApproval() throws Exception {
- testImplicitApprovalOnGetStatus(
- /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
- }
-
- private void testImplicitApprovalOnGetStatus(
- boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
- TestAccount projectOwner = admin;
-
- // Create change with a user that is not a project owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
-
- if (uploaderMatchesChangeOwner) {
- amendChange(projectOwner, changeId);
- } else {
- amendChange(user, changeId);
- }
-
- FileCodeOwnerStatus expectedFileCodeOwnerStatus;
- if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.APPROVED,
- String.format(
- "implicitly approved by the patch set uploader %s who is a fallback code"
- + " owner (all project owners are fallback code owners)",
- AccountTemplateUtil.getAccountTemplate(projectOwner.id())));
- } else {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses).containsExactly(expectedFileCodeOwnerStatus);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatus_noImplicitlyApprovalByPatchSetUploaderThatDoesntOwnTheChange()
- throws Exception {
- TestAccount admin2 = accountCreator.admin2();
-
- // Create change with a user that is a project owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
-
- // Amend change with a user that is another project owner.
- amendChange(admin2, changeId);
-
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
- public void getStatus_overrideApprovesAllFiles() throws Exception {
- createOwnersOverrideLabel();
-
- // Create a change with a user that is not a project owner.
- TestRepository<InMemoryRepository> testRepo = cloneProject(project, user);
- String path1 = "bar/baz.config";
- String path2 = "foo/baz.config";
- String changeId =
- pushFactory
- .create(
- user.newIdent(),
- testRepo,
- "Test Change",
- ImmutableMap.of(
- path2, "content",
- path1, "other content"))
- .to("refs/for/master")
- .getChangeId();
-
- // Without Owners-Override approval the expected status is INSUFFICIENT_REVIEWERS.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Add an override approval.
- gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
-
- // With Owners-Override approval the expected status is APPROVED.
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path1,
- CodeOwnerStatus.APPROVED,
- String.format(
- "override approval Owners-Override+1 by %s is present",
- AccountTemplateUtil.getAccountTemplate(admin.id()))),
- FileCodeOwnerStatus.addition(
- path2,
- CodeOwnerStatus.APPROVED,
- String.format(
- "override approval Owners-Override+1 by %s is present",
- AccountTemplateUtil.getAccountTemplate(admin.id()))));
- }
-
- @Test
- @GerritConfig(
- name = "plugin.code-owners.overrideApproval",
- values = {"Owners-Override+1", "Another-Override+1"})
- public void getStatus_anyOverrideApprovesAllFiles() throws Exception {
- createOwnersOverrideLabel();
- createOwnersOverrideLabel("Another-Override");
-
- // Create a change with a user that is not a project owner.
- TestRepository<InMemoryRepository> testRepo = cloneProject(project, user);
- String path1 = "bar/baz.config";
- String path2 = "foo/baz.config";
- String changeId =
- pushFactory
- .create(
- user.newIdent(),
- testRepo,
- "Test Change",
- ImmutableMap.of(
- path2, "content",
- path1, "other content"))
- .to("refs/for/master")
- .getChangeId();
-
- // Without override approval the expected status is INSUFFICIENT_REVIEWERS.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(path2, 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.
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path1,
- CodeOwnerStatus.APPROVED,
- String.format(
- "override approval Owners-Override+1 by %s is present",
- AccountTemplateUtil.getAccountTemplate(user.id()))),
- FileCodeOwnerStatus.addition(
- path2,
- CodeOwnerStatus.APPROVED,
- String.format(
- "override approval Owners-Override+1 by %s is present",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
-
- // 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.
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(path2, 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.
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path1,
- CodeOwnerStatus.APPROVED,
- String.format(
- "override approval Another-Override+1 by %s is present",
- AccountTemplateUtil.getAccountTemplate(user.id()))),
- FileCodeOwnerStatus.addition(
- path2,
- CodeOwnerStatus.APPROVED,
- String.format(
- "override approval Another-Override+1 by %s is present",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
- }
-
- @Test
- public void projectOwnersAreNotCodeOwnersIfDefaultCodeOwnerConfigExists() throws Exception {
- TestAccount user2 = accountCreator.user2();
-
- setAsDefaultCodeOwners(user);
-
- // Create a change as a user that is neither a code owner nor a project owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange(user2, "Change Adding A File", JgitPath.of(path).get(), "file content")
- .getChangeId();
-
- // Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Let the project owner approve the change.
- requestScopeOperations.setApiUser(admin.id());
- approve(changeId);
-
- // Verify that the file is not approved yet
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Let the code owner approve the change.
- projectOperations
- .project(project)
- .forUpdate()
- .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
- .update();
- requestScopeOperations.setApiUser(user.id());
- approve(changeId);
-
- // Check that the file is approved now.
- requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(
- path,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a default code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
- }
-
- private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
- throws Exception {
- return codeOwnerApprovalCheck.getFileStatusesAsSet(
- getChangeNotes(changeId), /* start= */ 0, /* limit= */ 0);
- }
-
- private ChangeNotes getChangeNotes(String changeId) throws Exception {
- return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
- }
-}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
index df5ad05..0422551 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
@@ -89,17 +89,13 @@
}
@Test
- public void cannotVisitCodeOwnerConfigsForNullRevision() throws Exception {
- NullPointerException npe =
- assertThrows(
- NullPointerException.class,
- () ->
- codeOwnerConfigHierarchy.visit(
- BranchNameKey.create(project, "master"),
- /* revision= */ null,
- Paths.get("/foo/bar/baz.md"),
- visitor));
- assertThat(npe).hasMessageThat().isEqualTo("revision");
+ public void visitorNotInvokedForNullRevision() throws Exception {
+ codeOwnerConfigHierarchy.visit(
+ BranchNameKey.create(project, "master"),
+ /* revision= */ null,
+ Paths.get("/foo/bar/baz.md"),
+ visitor);
+ verifyNoInteractions(visitor);
}
@Test
@@ -583,6 +579,27 @@
}
@Test
+ public void visitorInvokedForCodeOwnerConfigInRefsMetaConfig_nullRevision() throws Exception {
+ CodeOwnerConfig.Key metaCodeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch(RefNames.REFS_CONFIG)
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ when(visitor.visit(any(CodeOwnerConfig.class))).thenReturn(true);
+ codeOwnerConfigHierarchy.visit(
+ BranchNameKey.create(project, "master"),
+ /* revision= */ null,
+ Paths.get("/foo/bar/baz.md"),
+ visitor);
+ verify(visitor).visit(codeOwnerConfigOperations.codeOwnerConfig(metaCodeOwnerConfigKey).get());
+ verifyNoMoreInteractions(visitor);
+ }
+
+ @Test
public void visitorInvokedForCodeOwnerConfigInRefsMetaConfigIfItDoesntApply() throws Exception {
CodeOwnerConfig.Key metaCodeOwnerConfigKey =
codeOwnerConfigOperations
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
index a06db05..a2470e6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
@@ -93,7 +93,7 @@
LegacySubmitRequirementSubject submitRequirementSubject =
submitRecordSubject.hasSubmitRequirementsThat().onlyElement();
submitRequirementSubject.hasTypeThat().isEqualTo("code-owners");
- submitRequirementSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
}
@Test
@@ -119,7 +119,7 @@
LegacySubmitRequirementSubject submitRequirementSubject =
submitRecordSubject.hasSubmitRequirementsThat().onlyElement();
submitRequirementSubject.hasTypeThat().isEqualTo("code-owners");
- submitRequirementSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
}
@Test
@@ -228,6 +228,6 @@
LegacySubmitRequirementSubject submitRequirementSubject =
submitRecordSubject.hasSubmitRequirementsThat().onlyElement();
submitRequirementSubject.hasTypeThat().isEqualTo("code-owners");
- submitRequirementSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementSubject.hasFallbackTextThat().isEqualTo("Code-Owners");
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
index 1c0f9c1..e1786d3 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
@@ -49,10 +49,14 @@
assertThat(skipRetryWithTrace(newInvalidPathException())).isTrue();
assertThat(skipRetryWithTrace(newExceptionWithCause(newInvalidPathException()))).isTrue();
- assertThat(skipRetryWithTrace(new CodeOwnersInternalServerErrorException("msg"))).isFalse();
assertThat(
skipRetryWithTrace(
- newExceptionWithCause(new CodeOwnersInternalServerErrorException("msg"))))
+ CodeOwnersInternalServerErrorException.newInternalServerError("msg")))
+ .isFalse();
+ assertThat(
+ skipRetryWithTrace(
+ newExceptionWithCause(
+ CodeOwnersInternalServerErrorException.newInternalServerError("msg"))))
.isFalse();
assertThat(skipRetryWithTrace(new Exception())).isFalse();
@@ -82,7 +86,7 @@
.containsExactly(invalidPathException.getMessage());
CodeOwnersInternalServerErrorException codeOwnersInternalServerErrorException =
- new CodeOwnersInternalServerErrorException("msg");
+ CodeOwnersInternalServerErrorException.newInternalServerError("msg");
assertThat(getUserMessages(codeOwnersInternalServerErrorException))
.containsExactly(codeOwnersInternalServerErrorException.getUserVisibleMessage());
assertThat(getUserMessages(newExceptionWithCause(codeOwnersInternalServerErrorException)))
@@ -130,8 +134,12 @@
assertThat(getStatus(new Exception())).isEmpty();
assertThat(getStatus(newExceptionWithCause(new Exception()))).isEmpty();
- assertThat(getStatus(new CodeOwnersInternalServerErrorException("msg"))).isEmpty();
- assertThat(getStatus(newExceptionWithCause(new CodeOwnersInternalServerErrorException("msg"))))
+ assertThat(getStatus(CodeOwnersInternalServerErrorException.newInternalServerError("msg")))
+ .isEmpty();
+ assertThat(
+ getStatus(
+ newExceptionWithCause(
+ CodeOwnersInternalServerErrorException.newInternalServerError("msg"))))
.isEmpty();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorExceptionTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorExceptionTest.java
new file mode 100644
index 0000000..ac1e583
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorExceptionTest.java
@@ -0,0 +1,49 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
+
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnersInternalServerErrorException}. */
+public class CodeOwnersInternalServerErrorExceptionTest extends AbstractCodeOwnersTest {
+ @Test
+ public void codeOwnersInternalServerErrorExceptionIsCreatedByDefault() {
+ assertThat(newInternalServerError("foo", new NullPointerException("bar")))
+ .isInstanceOf(CodeOwnersInternalServerErrorException.class);
+ assertThat(
+ newInternalServerError("foo", newExceptionWithCause(new NullPointerException("bar"))))
+ .isInstanceOf(CodeOwnersInternalServerErrorException.class);
+ }
+
+ @Test
+ public void storageExceptionIsCreatedForNonCodeOwnerErrors() {
+ assertThat(newInternalServerError("foo", new DiffNotAvailableException("bar")))
+ .isInstanceOf(StorageException.class);
+ assertThat(
+ newInternalServerError(
+ "foo", newExceptionWithCause(new DiffNotAvailableException("bar"))))
+ .isInstanceOf(StorageException.class);
+ }
+
+ private Exception newExceptionWithCause(Exception cause) {
+ return new Exception("exception1", new Exception("exception2", cause));
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigTest.java
index 9ca0339..c972af9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigTest.java
@@ -471,6 +471,6 @@
.message("Configure code owner backend")
.add("code-owners.config", String.format("[%s \"%s\"]", SECTION, subsection)));
}
- projectCache.evict(project);
+ projectCache.evictAndReindex(project);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshotTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshotTest.java
index 1dbe420..66649f9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshotTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshotTest.java
@@ -1607,6 +1607,104 @@
}
@Test
+ public void cannotGetCodeOwnerConfigValidationPolicyForBranchCreationForNullBranch()
+ throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ cfgSnapshot()
+ .getCodeOwnerConfigValidationPolicyForBranchCreation(/* branchName= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("branchName");
+ }
+
+ @Test
+ public void getCodeOwnerConfigValidationPolicyForBranchCreation_notConfigured() throws Exception {
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("non-existing"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ public void getCodeOwnerConfigValidationPolicyForBranchCreation_configuredOnProjectLevel()
+ throws Exception {
+ configureEnableValidationOnBranchCreation(project, CodeOwnerConfigValidationPolicy.FALSE);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("non-existing"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ public void getCodeOwnerConfigValidationPolicyForBranchCreation_configuredOnBranchLevel()
+ throws Exception {
+ configureEnableValidationOnBranchCreationForBranch(
+ project, "refs/heads/master", CodeOwnerConfigValidationPolicy.TRUE);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE);
+ assertThat(
+ cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("refs/heads/master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("foo"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ public void getCodeOwnerConfigValidationPolicyForBranchCreation_branchLevelConfigTakesPrecedence()
+ throws Exception {
+ updateCodeOwnersConfig(
+ project,
+ codeOwnersConfig -> {
+ codeOwnersConfig.setEnum(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ GeneralConfig.KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ CodeOwnerConfigValidationPolicy.DRY_RUN);
+ codeOwnersConfig.setEnum(
+ GeneralConfig.SECTION_VALIDATION,
+ "refs/heads/master",
+ GeneralConfig.KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ CodeOwnerConfigValidationPolicy.FALSE);
+ });
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ assertThat(
+ cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("refs/heads/master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("foo"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.DRY_RUN);
+ }
+
+ @Test
+ public void
+ getCodeOwnerConfigValidationPolicyForBranchCreation_inheritedBranchLevelConfigTakesPrecedence()
+ throws Exception {
+ configureEnableValidationOnBranchCreationForBranch(
+ allProjects, "refs/heads/master", CodeOwnerConfigValidationPolicy.FALSE);
+ configureEnableValidationOnBranchCreation(project, CodeOwnerConfigValidationPolicy.DRY_RUN);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ assertThat(
+ cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("refs/heads/master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("foo"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.DRY_RUN);
+ }
+
+ @Test
+ public void
+ getCodeOwnerConfigValidationPolicyForBranchCreation_inheritedBranchLevelCanBeOverridden()
+ throws Exception {
+ configureEnableValidationOnBranchCreationForBranch(
+ allProjects, "refs/heads/master", CodeOwnerConfigValidationPolicy.FALSE);
+ configureEnableValidationOnBranchCreationForBranch(
+ project, "refs/heads/master", CodeOwnerConfigValidationPolicy.DRY_RUN);
+ assertThat(cfgSnapshot().getCodeOwnerConfigValidationPolicyForBranchCreation("master"))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.DRY_RUN);
+ }
+
+ @Test
public void cannotGetCodeOwnerConfigValidationPolicyForCommitReceivedForNullBranch()
throws Exception {
NullPointerException npe =
@@ -2036,6 +2134,31 @@
requiredApproval);
}
+ private void configureEnableValidationOnBranchCreation(
+ Project.NameKey project, CodeOwnerConfigValidationPolicy codeOwnerConfigValidationPolicy)
+ throws Exception {
+ setCodeOwnersConfig(
+ project,
+ /* subsection= */ null,
+ GeneralConfig.KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ codeOwnerConfigValidationPolicy.name());
+ }
+
+ private void configureEnableValidationOnBranchCreationForBranch(
+ Project.NameKey project,
+ String branchSubsection,
+ CodeOwnerConfigValidationPolicy codeOwnerConfigValidationPolicy)
+ throws Exception {
+ updateCodeOwnersConfig(
+ project,
+ codeOwnersConfig ->
+ codeOwnersConfig.setString(
+ GeneralConfig.SECTION_VALIDATION,
+ branchSubsection,
+ GeneralConfig.KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ codeOwnerConfigValidationPolicy.name()));
+ }
+
private void configureEnableValidationOnCommitReceived(
Project.NameKey project, CodeOwnerConfigValidationPolicy codeOwnerConfigValidationPolicy)
throws Exception {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
index 51011f0..86801cc 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_ADD_REVIEWER;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_CODE_OWNER_CONFIG_FILES_WITH_FILE_EXTENSIONS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_SUBMIT;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_EXEMPTED_USER;
@@ -584,7 +585,7 @@
throws Exception {
Config cfg = new Config();
cfg.setString(
- SECTION_CODE_OWNERS, /* scubsection= */ null, KEY_REJECT_NON_RESOLVABLE_IMPORTS, "true");
+ SECTION_CODE_OWNERS, /* subsection= */ null, KEY_REJECT_NON_RESOLVABLE_IMPORTS, "true");
assertThat(generalConfig.getRejectNonResolvableImports(project, cfg)).isTrue();
}
@@ -786,6 +787,234 @@
}
@Test
+ public void cannotGetEnableValidationOnBranchCreationForNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreation(
+ project, /* pluginConfig= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void noEnableValidationOnBranchCreationConfiguration() throws Exception {
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreation(
+ project, new Config()))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "true")
+ public void
+ enableValidationOnBranchCreationConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
+ throws Exception {
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreation(
+ project, new Config()))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "false")
+ public void
+ enableValidationOnBranchConfigurationInPluginConfigOverridesEnableValidationOnBranchCreationConfigurationInGerritConfig()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ "true");
+ assertThat(generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreation(project, cfg))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "false")
+ public void invalidEnableValidationOnBranchCreationConfigurationInPluginConfigIsIgnored()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ "INVALID");
+ assertThat(generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreation(project, cfg))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnBranchCreation", value = "INVALID")
+ public void invalidEnableValidationOnBranchCreationConfigurationInGerritConfigIsIgnored()
+ throws Exception {
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreation(
+ project, new Config()))
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ public void cannotGetEnableValidationOnBranchForBranchForNullBranch() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ /* branchNameKey= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("branchNameKey");
+ }
+
+ @Test
+ public void cannotGetEnableValidationOnBranchCreationForBranchForNullPluginConfig()
+ throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), /* pluginConfig= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void noBranchSpecificEnableValidationOnBranchCreationConfiguration() throws Exception {
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), new Config()))
+ .isEmpty();
+ }
+
+ @Test
+ public void noMatchingBranchSpecificEnableValidationOnBranchCreationConfiguration_exact()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION, "refs/heads/foo", KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION, "false");
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), cfg))
+ .isEmpty();
+ }
+
+ @Test
+ public void noMatchingBranchSpecificEnableValidationOnBranchCreationConfiguration_refPattern()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION, "refs/heads/foo/*", KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED, "false");
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), cfg))
+ .isEmpty();
+ }
+
+ @Test
+ public void noMatchingBranchSpecificEnableValidationOnBranchCreationConfiguration_regEx()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION,
+ "^refs/heads/.*foo.*",
+ KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ "false");
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), cfg))
+ .isEmpty();
+ }
+
+ @Test
+ public void noMatchingBranchSpecificEnableValidationOnBranchCreationConfiguration_invalidRegEx()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION, "^refs/heads/[", KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION, "false");
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), cfg))
+ .isEmpty();
+ }
+
+ @Test
+ public void matchingBranchSpecificEnableValidationOnBranchCreationConfiguration_exact()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION, "refs/heads/master", KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION, "false");
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), cfg))
+ .value()
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ public void matchingBranchSpecificEnableValidationOnBranchCreationConfiguration_refPattern()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION, "refs/heads/*", KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION, "false");
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), cfg))
+ .value()
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ public void matchingBranchSpecificEnableValidationOnBranchCreationConfiguration_regEx()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION,
+ "^refs/heads/.*bar.*",
+ KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ "false");
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "foobarbaz"), cfg))
+ .value()
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
+ public void branchSpecificEnableValidationOnBranchCreationConfigurationIsIgnoredIfValueIsInvalid()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION,
+ "refs/heads/master",
+ KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION,
+ "INVALID");
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), cfg))
+ .isEmpty();
+ }
+
+ @Test
+ public void multipleMatchingBranchSpecificEnableValidationOnBranchCreationConfiguration()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_VALIDATION, "refs/heads/master", KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION, "false");
+ cfg.setString(
+ SECTION_VALIDATION, "refs/heads/*", KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION, "false");
+ cfg.setString(
+ SECTION_VALIDATION, "^refs/heads/.*", KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION, "false");
+
+ // it is non-deterministic which of the branch-specific configurations takes precedence, but
+ // since they all configure the same value it's not important for this assertion
+ assertThat(
+ generalConfig.getCodeOwnerConfigValidationPolicyForBranchCreationForBranch(
+ BranchNameKey.create(project, "master"), cfg))
+ .value()
+ .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE);
+ }
+
+ @Test
public void cannotGetEnableValidationOnCommitReceivedForNullPluginConfig() throws Exception {
NullPointerException npe =
assertThrows(
@@ -1296,7 +1525,7 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> generalConfig.validateProjectLevelConfig(/*project= */ null, new Config()));
+ () -> generalConfig.validateProjectLevelConfig(/* fileName= */ null, new Config()));
assertThat(npe).hasMessageThat().isEqualTo("fileName");
}
@@ -1307,7 +1536,7 @@
NullPointerException.class,
() ->
generalConfig.validateProjectLevelConfig(
- "code-owners.config", /* pluginConfig= */ null));
+ "code-owners.config", /* projectLevelConfig= */ null));
assertThat(npe).hasMessageThat().isEqualTo("projectLevelConfig");
}
@@ -1687,6 +1916,14 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
+ public void defaultValueUsedIfRemovedProjectOwnersOptionIsSetForFallbackCodeOwners()
+ throws Exception {
+ assertThat(generalConfig.getFallbackCodeOwners(project, new Config()))
+ .isEqualTo(FallbackCodeOwners.NONE);
+ }
+
+ @Test
public void cannotGetMaxPathsInChangeMessagesForNullProject() throws Exception {
NullPointerException npe =
assertThrows(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/RequiredApprovalTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/RequiredApprovalTest.java
index ef2e43d..3979c48 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/RequiredApprovalTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/RequiredApprovalTest.java
@@ -211,7 +211,7 @@
return PatchSetApproval.builder()
.key(PatchSetApproval.key(PatchSet.id(Change.id(1), 1), admin.id(), labelType.getLabelId()))
.value(value)
- .granted(TimeUtil.nowTs())
+ .granted(TimeUtil.now())
.build();
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
index ae42bcd..260e635 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
@@ -924,7 +924,6 @@
private static void assertSplitGlobs(String commaSeparatedGlobs, String... expectedGlobs) {
assertThat(FindOwnersCodeOwnerConfigParser.Parser.splitGlobs(commaSeparatedGlobs))
- .asList()
.containsExactlyElementsIn(expectedGlobs);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackendTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackendTest.java
index 01d7091..9714196 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackendTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackendTest.java
@@ -1,3 +1,17 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
package com.google.gerrit.plugins.codeowners.backend.proto;
import static com.google.gerrit.truth.OptionalSubject.assertThat;
diff --git a/resources/Documentation/backend-find-owners.md b/resources/Documentation/backend-find-owners.md
index 18fb11e..9d2fab5 100644
--- a/resources/Documentation/backend-find-owners.md
+++ b/resources/Documentation/backend-find-owners.md
@@ -167,6 +167,7 @@
file](backends.html#codeOwnerConfigFiles). It's not possible to import arbitrary
files.
+##### <a id="referenceCodeOwnerConfigFilesFromOtherProjects">
It's also possible to reference code owner config files from other projects or
branches (only within the same host):
@@ -312,8 +313,9 @@
### <a id="anotations">Annotations
-Lines representing [access grants](#accessGrants) can be annotated. Annotations
-have the format `#{ANNOTATION_NAME}` and can appear at the end of the line.
+Lines that assign code ownership to users ([email lines](#userEmails) and
+[per-file lines](#perFile)) can be annotated. Annotations have the format
+`#{ANNOTATION_NAME}` and can appear at the end of the line.
E.g.:
```
@@ -343,9 +345,14 @@
Unknown annotations are silently ignored.
-**NOTE:** If an access grant line that assigns code ownership to multiple users
-has an annotation, this annotation applies to all these users. E.g. if an
-annotation is set for the all users wildcard (aka `*`) it applies to all users.
+**NOTE:** If a line that assigns code ownership to multiple users has an
+annotation, this annotation applies to all these users. E.g. if an annotation is
+set for the all users wildcard (aka `*`) it applies to all users.
+
+**NOTE:** Only [email lines](#userEmails) and [per-file lines](#perFile) support
+annotations, for other lines (e.g. [file lines](#fileKeyword) and [include
+lines](#includeKeyword)) annotations are interpreted as [comments](#comments)
+and are silently ignored.
### <a id="comments">Comments
diff --git a/resources/Documentation/config-faqs.md b/resources/Documentation/config-faqs.md
index ab88bb7..8766ffd 100644
--- a/resources/Documentation/config-faqs.md
+++ b/resources/Documentation/config-faqs.md
@@ -5,6 +5,8 @@
* [How to avoid issues with code owner config files](#avoidIssuesWithCodeOwnerConfigs)
* [How to investigate issues with code owner config files](#investigateIssuesWithCodeOwnerConfigs)
* [How to investigate issues with the code owner suggestion](#investigateIssuesWithCodeOwnerSuggestion)
+* [What should be done when creating a branch fails due to invalid code owner
+ config files?](#branchCreationFailsDueInvalidCodeOwnerConfigFiles)
* [How to define default code owners](#defineDefaultCodeOwners)
* [How to setup code owner overrides](#setupOverrides)
* [What's the best place to keep the global plugin
@@ -136,6 +138,31 @@
Also see [above](#avoidIssuesWithCodeOwnerConfigs) how to avoid issues with code
owner config files in the first place.
+## <a id="branchCreationFailsDueInvalidCodeOwnerConfigFiles">What should be done when creating a branch fails due to invalid code owner config files?
+
+When creating a new branch, all code owner config files that are contained in
+the initial commit are newly [validated](validation.html#codeOwnerConfigValidationOnBranchCreation), even if the branch is created for a
+commit that already exists in the repository.
+
+If creating a branch fails due to this validation, it is recommended to:
+
+1. Use the [code-owners~skip-validation
+ validation](validation.html#skipCodeOwnerConfigValidationOnDemand) option to
+ skip the validation of code owner config files when creating the branch.
+2. Use the
+ [Check Code Owner Config Files](rest-api.html#check-code-owner-config-files)
+ REST endpoint to validate the code owner files in the new branch (specify
+ the branch in the `branches` field in the
+ [CheckCodeOwnerConfigFilesInput](rest-api.html#check-code-owner-config-files-input))
+ to see which code owner config files have issues.
+3. Fix the reported issues and push them as a change for code review. If
+ needed, get the change submitted with a [code owner
+ override](user-guide.html#codeOwnerOverride).
+4. Repeat step 2. to verify that all issues have been fixed.
+
+It's also possible to switch of the code owner config validation on branch
+creation by [configuration](config.html#pluginCodeOwnersEnableValidationOnBranchCreation).
+
## <a id="defineDefaultCodeOwners">How to define default code owners
[Default code owners](backend-find-owners.html#defaultCodeOwnerConfiguration)
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
index e9d5b88..0975cff 100644
--- a/resources/Documentation/config-guide.md
+++ b/resources/Documentation/config-guide.md
@@ -249,6 +249,39 @@
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.
+## <a id="configureCodeOwnersByPermissions">How to configure code owners without the @PLUGIN@ plugin when all files / folders are owned by the same users
+
+If all files / folders in a repository / branch are owned by the same users, using the
+`@PLUGIN@` plugin is not needed and not recommended. Instead you should use
+plain [Gerrit permissions](../../../Documentation/access-control.html) to
+control who can approve changes:
+
+* Configure the `Code-Review` label with a voting range from `-2` to `+2` (see
+ [label configuration](../../../Documentation/config-labels.html#label_Code-Review).
+* For the code owners assign permissions to allow voting from `Code-Review-2` to
+ `Code-Review+2` and allow other users only to vote from `Code-Review-1` to
+ `Code-Review+1` (see [label
+ permissions](../../../Documentation/access-control.html#category_review_labels)
+* Setup a submit requirement that requires a `Code-Review+2` approval for making
+ the change submittable, optionally with disallowing self approvals (see
+ [submit
+ requirements](../../../Documentation/config-submit-requirements.html)).
+
+With this configuration a `Code-Review+2` approval from a code owner is required
+for changes to become submittable.
+
+Advantages of this approach over using the `@PLUGIN@` plugin:
+* it requires less configuration as you do not need to maintain code owner
+ config files (aka `OWNERS` files)
+* groups are supported (permissions can be assigned to groups, but code
+ ownerwhip in code owner config files [cannot be assigned to
+ groups](backend-find-owners-cookbook.html#defineAGroupAsCodeOwner))
+* it's more performant since code owners do not need to be computed in order to
+ detect if a change is submittable
+
+Disadvantages:
+* there are no code owner suggestions and no code owner specific UI controls
+
---
Back to [@PLUGIN@ documentation index](index.html)
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 308ca8f..b4ea424 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -304,6 +304,50 @@
`@PLUGIN@.config`.\
By default `false`.
+<a id="pluginCodeOwnersEnableValidationOnBranchCreation">plugin.@PLUGIN@.enableValidationOnBranchCreation</a>
+: Policy for validating code owner config files when a branch is created.
+ If the validation is on all code owner config files that are contained
+ in the commit on which the branch is being created are validated. Doing
+ this is rather expensive and will make branch creations significantly
+ slower (in average a latency increase of 10s to 20s is expected).
+ \
+ Can be `TRUE`, `FALSE`, `DRY_RUN`, `FORCED` or `FORCED_DRY_RUN`.\
+ \
+ `TRUE`:\
+ The code owner config file validation is enabled and the creation of
+ branches that contain invalid code owner config files is rejected.\
+ If the code owners functionality is disabled, no validation is
+ performed.\
+ \
+ `FALSE`:\
+ The code owner config file validation is disabled, the creation of
+ branches that contain invalid code owner config files is not rejected.\
+ \
+ `DRY_RUN`:\
+ Code owner config files are validated on branch creation, but the
+ creation of branches that contain invalid code owner config files is not
+ rejected.\
+ If the code owners functionality is disabled, no dry-run validation is
+ performed.\
+ \
+ `FORCED`:\
+ Code owner config files are validated on branch creation even if the
+ code owners functionality is disabled.\
+ This option is useful when the code owner config validation should be
+ enabled as preparation to enabling the code owners functionality.\
+ \
+ `FORCED_DRY_RUN`:\
+ Code owner config files are validated on branch creation even if the
+ code owners functionality is disabled, but the creation of branches that
+ contain invalid code owner config files is not rejected.\
+ This option is useful when the code owner config validation should be
+ enabled as preparation to enabling the code owners functionality.\
+ \
+ Can be overridden per project by setting
+ [codeOwners.enableValidationOnBranchCreation](#codeOwnersEnableValidationOnBranchCreation)
+ in `@PLUGIN@.config`.\
+ By default `FALSE`.
+
<a id="pluginCodeOwnersEnableValidationOnCommitReceived">plugin.@PLUGIN@.enableValidationOnCommitReceived</a>
: Policy for validating code owner config files when a commit is received.
\
@@ -530,18 +574,13 @@
owners hasn't been explicity disabled in a relevant code owner config
file and if there are no unresolved imports.\
\
- Can be `NONE`, `PROJECT_OWNERS` or `ALL_USERS`.\
+ Can be `NONE` or `ALL_USERS`.\
\
`NONE`:\
Paths for which no code owners are defined are owned by no one. This
means changes that touch these files can only be submitted with a code
owner override.\
\
- `PROJECT_OWNERS`:\
- Paths for which no code owners are defined are owned by the project
- owners. This means changes to these paths can be approved by the project
- owners.\
- \
`ALL_USERS`:\
Paths for which no code owners are defined are owned by all users. This
means changes to these paths can be approved by anyone. If [implicit
@@ -877,6 +916,35 @@
[plugin.@PLUGIN@.exemptPureReverts](#pluginCodeOwnersExemptPureReverts)
in `gerrit.config` is used.
+<a id="codeOwnersEnableValidationOnBranchCreation">codeOwners.enableValidationOnBranchCreation</a>
+: Policy for validating code owner config files when a branch is created.\
+ Can be `TRUE`, `FALSE`, `DRY_RUN`, `FORCED` or `FORCED_DRY_RUN`. For a
+ description of the values see
+ [plugin.@PLUGIN@.enableValidationOnBranchCreation](#pluginCodeOwnersEnableValidationOnBranchCreation).\
+ Overrides the global setting
+ [plugin.@PLUGIN@.enableValidationOnBranchCreation](#pluginCodeOwnersEnableValidationOnBranchCreation)
+ in `gerrit.config` and the `codeOwners.enableValidationOnBranchCreation`
+ setting from parent projects.\
+ Can be overriden on branch-level by setting
+ [validation.\<branch\>.enableValidationOnBranchCreation](#validationBranchEnableValidationOnBranchCreation).\
+ If not set, the global setting
+ [plugin.@PLUGIN@.enableValidationOnBranchCreation](#pluginCodeOwnersEnableValidationOnBranchCreation)
+ in `gerrit.config` is used.
+
+<a id="validationBranchEnableValidationOnBranchCreation">validation.\<branch\>.enableValidationOnBranchCreation</a>
+: Branch-level policy for validating code owner config files when a branch
+ is created.\
+ Applies to all branches that are matched by `<branch>`, which can be
+ an exact ref name (e.g. `refs/heads/master`), a ref pattern (e.g.
+ `refs/heads/*`) or a regular expression (e.g. `^refs/heads/stable-.*`).\
+ If a branch matches multiple validation subsections it is undefined
+ which of the subsections takes precedence.\
+ Overrides the project-level configuration for validating code owner
+ config files when a branch is created that is configured by
+ [codeOwners.enableValidationOnBranchCreation](#codeOwnersEnableValidationOnBranchCreation).\
+ For further details see the description of
+ [codeOwners.enableValidationOnBranchCreation](#codeOwnersEnableValidationOnBranchCreation).
+
<a id="codeOwnersEnableValidationOnCommitReceived">codeOwners.enableValidationOnCommitReceived</a>
: Policy for validating code owner config files when a commit is
received.\
@@ -1084,7 +1152,7 @@
defined. This policy only applies if the inheritance of parent code
owners hasn't been explicity disabled in a relevant code owner config
file and if there are no unresolved imports.\
- Can be `NONE`, `PROJECT_OWNERS` or `ALL_USERS` (see
+ Can be `NONE` or `ALL_USERS` (see
[plugin.@PLUGIN@.fallbackCodeOwners](#pluginCodeOwnersFallbackCodeOwners)
for an explanation of these values).\
Overrides the global setting
diff --git a/resources/Documentation/how-to-use.md b/resources/Documentation/how-to-use.md
index 4d7a2d9..552f675 100644
--- a/resources/Documentation/how-to-use.md
+++ b/resources/Documentation/how-to-use.md
@@ -3,6 +3,13 @@
The `@PLUGIN@` plugin provides support for
[code owners](user-guide.html#codeOwners) in Gerrit.
+**NOTE:** You would want to use the `@PLUGIN@` plugin if you have a repository /
+branch that contains files / folders that are owned by different users. If this
+is not the case, and all files / folders in the repository / branch are owned by
+the same users, using the `@PLUGIN@` plugin is not recommended, and [using plain
+Gerrit permissions should be preferred
+instead](config-guide.html#configureCodeOwnersByPermissions).
+
If the `@PLUGIN@` plugin is enabled, changes can only be submitted if all
touched files are covered by [approvals](user-guide.html#codeOwnerApproval) from
code owners.
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index d42cf50..0151f7a 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -986,7 +986,7 @@
| `file_extension` | optional | The file extension that should be used for code owner config files in this project.
| `required_approval` | optional | The approval that is required from code owners. Must be specified in the format "\<label-name\>+\<label-value\>". If an empty string is provided the required approval configuration is unset. Unsetting the required approval means that the inherited required approval configuration or the default required approval (`Code-Review+1`) will apply. In contrast to providing an empty string, providing `null` (or not setting the value) means that the required approval configuration is not updated.
| `override_approvals` | optional | The approvals that count as override for the code owners submit check. Must be specified in the format "\<label-name\>+\<label-value\>".
-| `fallback_code_owners` | optional | Policy that controls who should own paths that have no code owners defined. Possible values are: `NONE`: Paths for which no code owners are defined are owned by no one. `PROJECT_OWNERS`: Paths for which no code owners are defined are owned by the project owners. `ALL_USERS`: Paths for which no code owners are defined are owned by all users.
+| `fallback_code_owners` | optional | Policy that controls who should own paths that have no code owners defined. Possible values are: `NONE`: Paths for which no code owners are defined are owned by no one. `ALL_USERS`: Paths for which no code owners are defined are owned by all users.
| `global_code_owners` | optional | List of emails of users that should be code owners globally across all branches.
| `exempted_users` | optional | List of emails of users that should be exempted from requiring code owners approvals.
| `merge_commit_strategy` | optional | Strategy that defines for merge commits which files require code owner approvals. Can be `ALL_CHANGED_FILES` or `FILES_WITH_CONFLICT_RESOLUTION` (see [mergeCommitStrategy](config.html#pluginCodeOwnersMergeCommitStrategy) for an explanation of these values).
@@ -995,8 +995,9 @@
| `invalid_code_owner_config_info_url` | optional | URL for a page that provides project/host-specific information about how to deal with invalid code owner config files.
| `read_only` | optional | Whether code owner config files are read-only.
| `exempt_pure_reverts` | optional | Whether pure revert changes are exempted from needing code owner approvals for submit.
-| `enable_validation_on_commit_received` | optional | Policy for validating code owner config files when a commit is received. Allowed values are `true` (the code owner config file validation is enabled and the upload of invalid code owner config files is rejected), `false` (the code owner config file validation is disabled, invalid code owner config files are not rejected) and `dry_run` (code owner config files are validated, but invalid code owner config files are not rejected).
-| `enable_validation_on_submit` | optional | Policy for validating code owner config files when a change is submitted. Allowed values are `true` (the code owner config file validation is enabled and the submission of invalid code owner config files is rejected), `false` (the code owner config file validation is disabled, invalid code owner config files are not rejected) and `dry_run` (code owner config files are validated, but invalid code owner config files are not rejected).
+| `enable_validation_on_branch_creation` | optional | Policy for validating code owner config files when a branch is created. Allowed values are `true` (the code owner config file validation is enabled and the creation of branches that contain invalid code owner config files is rejected), `false` (the code owner config file validation is disabled, the creation of branches that contain invalid code owner config files is not rejected), `dry_run` (code owner config files are validated on branch creation, but the creation of branches that contain invalid code owner config files is not rejected), `forced` (code owner config files are validated on branch creation even if the code owners functionality is disabled) and `forced_dry_run` (code owner config files are validated on branch creation even if the code owners functionality is disabled, but the creation of branches that contain invalid code owner config files is not rejected).
+| `enable_validation_on_commit_received` | optional | Policy for validating code owner config files when a commit is received. Allowed values are `true` (the code owner config file validation is enabled and the upload of invalid code owner config files is rejected), `false` (the code owner config file validation is disabled, invalid code owner config files are not rejected), `dry_run` (code owner config files are validated, but invalid code owner config files are not rejected), `forced` (code owner config files are validated even if the code owners functionality is disabled) and `forced_dry_run` (code owner config files are validated even if the code owners functionality is disabled, but invalid code owner config files are not rejected).
+| `enable_validation_on_submit` | optional | Policy for validating code owner config files when a change is submitted. Allowed values are `true` (the code owner config file validation is enabled and the submission of invalid code owner config files is rejected), `false` (the code owner config file validation is disabled, invalid code owner config files are not rejected), `dry_run` (code owner config files are validated, but invalid code owner config files are not rejected), `forced` (code owner config files are validated even if the code owners functionality is disabled) and `forced_dry_run` (code owner config files are validated even if the code owners functionality is disabled, but invalid code owner config files are not rejected).
| `reject_non_resolvable_code_owners` | optional | Whether modifications of code owner config files that newly add non-resolvable code owners should be rejected on commit received and submit.
| `reject_non_resolvable_imports` | optional | Whether modifications of code owner config files that newly add non-resolvable imports should be rejected on commit received an submit.
| `max_paths_in_change_messages` | optional | The maximum number of paths that are included in change messages. Setting the value to `0` disables including owned paths into change messages.
@@ -1069,7 +1070,7 @@
| `implicit_approvals` | optional | Whether an implicit code owner approval from the last uploader is assumed (see [enableImplicitApprovals](config.html#pluginCodeOwnersEnableImplicitApprovals) for details). When unset, `false`.
| `override_info_url` | optional | Optional URL for a page that provides project/host-specific information about how to request a code owner override.
| `invalid_code_owner_config_info_url` | optional | Optional URL for a page that provides project/host-specific information about how to deal with invalid code owner config files.
-|`fallback_code_owners` || Policy that controls who should own paths that have no code owners defined. Possible values are: `NONE`: Paths for which no code owners are defined are owned by no one. `PROJECT_OWNERS`: Paths for which no code owners are defined are owned by the project owners. `ALL_USERS`: Paths for which no code owners are defined are owned by all users.
+|`fallback_code_owners` || Policy that controls who should own paths that have no code owners defined. Possible values are: `NONE`: Paths for which no code owners are defined are owned by no one. `ALL_USERS`: Paths for which no code owners are defined are owned by all users.
### <a id="owned-changed-file-info"> OwnedChangedFileInfo
The `OwnedChangedFileInfo` entity contains information about a file that was
@@ -1159,7 +1160,7 @@
Administrators have this capability implicitly assigned.
The same as all global capabilities, the `Check Code Owner` global capability is
-assigned on the `All-Project` project in the `Global Capabilities` access
+assigned on the `All-Projects` project in the `Global Capabilities` access
section.
---
diff --git a/resources/Documentation/setup-guide.md b/resources/Documentation/setup-guide.md
index 63aa7e0..15ed0d4 100644
--- a/resources/Documentation/setup-guide.md
+++ b/resources/Documentation/setup-guide.md
@@ -297,8 +297,8 @@
It is possible to configure a policy for [fallback code
owners](config.html#pluginCodeOwnersFallbackCodeOwners) that controls who should
-own files for which no code owners have been defined, e.g. project owners, all
-users or no one (default).
+own files for which no code owners have been defined, e.g. all users or no one
+(default).
Configuring fallback code owners is optional. For the initial rollout of the
code-owners plugin it is highly recommended to allow fallback code owners so
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md
index d7a5548..60ee4d2 100644
--- a/resources/Documentation/validation.md
+++ b/resources/Documentation/validation.md
@@ -95,17 +95,49 @@
validation that was done on upload. This means, all visibility checks will be
done from the perspective of the uploader.
+## <a id="codeOwnerConfigValidationOnBranchCreation">Code owner config validation on branch creation
+
+It's possible to [enable validation of code owner config files on branch
+creation](config.html#pluginCodeOwnersEnableValidationOnBranchCreation) (off by
+default).
+
+If the validation is enabled and a new branch is created, all code owner config
+files that are contained in the initial commit are newly validated, even if the
+branch is created for a commit that already exists in the repository.
+
+Validating code owner config files newly when a branch is created makes sense
+because:
+
+* the validation configuration of the new branch may differ from the validation
+ configuration of the branch that already contains the commit
+* [imports from other projects](backend-find-owners.html#referenceCodeOwnerConfigFilesFromOtherProjects)
+ that do not specify a branch may not be resolvable: If a branch is not
+ specified it's assumed that the code owner config file from the other project
+ should be imported from the same branch that contains the importing code owner
+ config. This means when a new branch `foo` is being created a code owner
+ config file that is referenced by such an import is expected to be found in
+ the branch `foo` of the other project, but this branch may not exist there so
+ that the import is unresolvable. By validating the code owner config files on
+ branch creation such unresolvable imports are detected and flagged.
+
+What should be done if the creation of a branch fails due to invalid code owner
+config files is explained in the
+[config FAQs](config-faqs.html#branchCreationFailsDueInvalidCodeOwnerConfigFiles).
+
## <a id="skipCodeOwnerConfigValidationOnDemand">Skip code owner config validation on demand
By setting the `code-owners~skip-validation` push option it is possible to skip
the code owner config validation on push:
`git push -o code-owners~skip-validation origin HEAD:refs/for/master`
-For the [Create Change](../../../Documentation/rest-api-changes.html#create-change)
-REST endpoint skipping the code owner config validation is possible by setting
+For the [Create
+Branch](../../../Documentation/rest-api-projects.html#create-branch), [Create
+Change](../../../Documentation/rest-api-changes.html#create-change), the [Cherry
+Pick Revision](../../../Documentation/rest-api-changes.html#cherry-pick) and the
+[Rebase](../../../Documentation/rest-api-changes.html#rebase-change) REST
+endpoints skipping the code owner config validation is possible by setting
`code-owners~skip-validation` with the value `true` as a validation option in
-the [ChangeInput](../../../Documentation/rest-api-changes.html#change-input)
-(see field `validation_options`).
+the input (see field `validation_options`).
Using the push option or the validation option requires the calling user to
have the `Can Skip Code Owner Config Validation` global capability. Host
diff --git a/test/index.html b/test/index.html
index 2273093..2ee74df 100644
--- a/test/index.html
+++ b/test/index.html
@@ -1,7 +1,7 @@
<!DOCTYPE html>
<!--
@license
-Copyright (C) 2018 The Android Open Source Project
+Copyright (C) 2020 The Android Open Source Project
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
@@ -25,4 +25,4 @@
WCT.loadSuites([
'../ui/code-owners-service_test.html'
]);
-</script>
\ No newline at end of file
+</script>
diff --git a/ui/code-owners-model.js b/ui/code-owners-model.js
index ad536b4..b6fcbbf 100644
--- a/ui/code-owners-model.js
+++ b/ui/code-owners-model.js
@@ -39,6 +39,19 @@
ALL_SUGGESTIONS: 'ALL_SUGGESTIONS',
};
+/**
+ * @enum
+ */
+export const UserRole = {
+ ANONYMOUS: 'ANONYMOUS',
+ AUTHOR: 'AUTHOR',
+ CHANGE_OWNER: 'CHANGE_OWNER',
+ REVIEWER: 'REVIEWER',
+ CC: 'CC',
+ REMOVED_REVIEWER: 'REMOVED_REVIEWER',
+ OTHER: 'OTHER',
+};
+
export const BestSuggestionsLimit = 5;
export const AllSuggestionsLimit = 1000;
diff --git a/ui/code-owners-service.js b/ui/code-owners-service.js
index f7ee6d2..3a5cbdc 100644
--- a/ui/code-owners-service.js
+++ b/ui/code-owners-service.js
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-import {SuggestionsType, BestSuggestionsLimit, AllSuggestionsLimit} from './code-owners-model.js';
+import {SuggestionsType, BestSuggestionsLimit, AllSuggestionsLimit, UserRole} from './code-owners-model.js';
import {OwnersProvider, OwnerStatus, FetchStatus} from './code-owners-fetcher.js';
import {CodeOwnersApi, CodeOwnersCacheApi} from './code-owners-api.js';
@@ -31,19 +31,6 @@
};
/**
- * @enum
- */
-const UserRole = {
- ANONYMOUS: 'ANONYMOUS',
- AUTHOR: 'AUTHOR',
- CHANGE_OWNER: 'CHANGE_OWNER',
- REVIEWER: 'REVIEWER',
- CC: 'CC',
- REMOVED_REVIEWER: 'REMOVED_REVIEWER',
- OTHER: 'OTHER',
-};
-
-/**
* Service for the data layer used in the plugin UI.
*/
export class CodeOwnerService {
diff --git a/ui/owner-requirement.js b/ui/owner-requirement.js
index 6ba53b6..c15fcf3 100644
--- a/ui/owner-requirement.js
+++ b/ui/owner-requirement.js
@@ -18,7 +18,7 @@
import {OwnerStatus} from './code-owners-fetcher.js';
import {CodeOwnersModelMixin} from './code-owners-model-mixin.js';
import {showPluginFailedMessage} from './code-owners-banner.js';
-import {isPluginErrorState} from './code-owners-model.js';
+import {isPluginErrorState, UserRole} from './code-owners-model.js';
/**
* Owner requirement control for `submit-requirement-item-code-owners` endpoint.
@@ -75,9 +75,11 @@
title="Documentation for overriding code owners"></iron-icon>
</a>
</template>
- <gr-button link on-click="_openReplyDialog">
- [[_getSuggestOwnersText(_statusCount)]]
- </gr-button>
+ <template is="dom-if" if="[[_isSignedInUser]]">
+ <gr-button link on-click="_openReplyDialog">
+ [[_getSuggestOwnersText(_statusCount)]]
+ </gr-button>
+ </template>
</template>
<template is="dom-if" if="[[_newerPatchsetUploaded]]">
<span>A newer patch set has been uploaded.</span>
@@ -118,6 +120,10 @@
type: String,
computed: '_computeOverrideInfoUrl(model.branchConfig)',
},
+ _isSignedInUser: {
+ type: Boolean,
+ computed: '_computeIsSignedInUser(model.userRole)'
+ }
};
}
@@ -142,6 +148,10 @@
return !branchConfig || !status || !userRole;
}
+ _computeIsSignedInUser(userRole) {
+ return userRole && userRole !== UserRole.ANONYMOUS;
+ }
+
_pluginFailed(pluginStatus) {
return pluginStatus && isPluginErrorState(pluginStatus.state);
}
