Merge "validaton.md: Link to the config options that disable some of the checks"
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index eee9cfb..6e59621 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -833,32 +833,38 @@
RevWalk revWalk,
Path codeOwnerConfigFilePath,
CodeOwnerConfig codeOwnerConfig) {
- return Streams.concat(
- codeOwnerConfig.imports().stream()
- .map(
- codeOwnerConfigReference ->
- validateCodeOwnerConfigReference(
- branchNameKey,
- revWalk,
- codeOwnerConfigFilePath,
- codeOwnerConfig.key(),
- codeOwnerConfig.revision(),
- CodeOwnerConfigImportType.GLOBAL,
- codeOwnerConfigReference)),
- codeOwnerConfig.codeOwnerSets().stream()
- .flatMap(codeOwnerSet -> codeOwnerSet.imports().stream())
- .map(
- codeOwnerConfigReference ->
- validateCodeOwnerConfigReference(
- branchNameKey,
- revWalk,
- codeOwnerConfigFilePath,
- codeOwnerConfig.key(),
- codeOwnerConfig.revision(),
- CodeOwnerConfigImportType.PER_FILE,
- codeOwnerConfigReference)))
- .filter(Optional::isPresent)
- .map(Optional::get);
+ try {
+ RevCommit codeOwnerConfigRevision = revWalk.parseCommit(codeOwnerConfig.revision());
+ return Streams.concat(
+ codeOwnerConfig.imports().stream()
+ .map(
+ codeOwnerConfigReference ->
+ validateCodeOwnerConfigReference(
+ branchNameKey,
+ revWalk,
+ codeOwnerConfigFilePath,
+ codeOwnerConfig.key(),
+ codeOwnerConfigRevision,
+ CodeOwnerConfigImportType.GLOBAL,
+ codeOwnerConfigReference)),
+ codeOwnerConfig.codeOwnerSets().stream()
+ .flatMap(codeOwnerSet -> codeOwnerSet.imports().stream())
+ .map(
+ codeOwnerConfigReference ->
+ validateCodeOwnerConfigReference(
+ branchNameKey,
+ revWalk,
+ codeOwnerConfigFilePath,
+ codeOwnerConfig.key(),
+ codeOwnerConfigRevision,
+ CodeOwnerConfigImportType.PER_FILE,
+ codeOwnerConfigReference)))
+ .filter(Optional::isPresent)
+ .map(Optional::get);
+ } catch (IOException e) {
+ throw new CodeOwnersInternalServerErrorException(
+ String.format("Failed to validate imports for %s in ", codeOwnerConfig.key()), e);
+ }
}
/**
@@ -881,7 +887,7 @@
RevWalk revWalk,
Path codeOwnerConfigFilePath,
CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
- ObjectId codeOwnerConfigRevision,
+ RevCommit codeOwnerConfigRevision,
CodeOwnerConfigImportType importType,
CodeOwnerConfigReference codeOwnerConfigReference) {
CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig =
@@ -902,16 +908,20 @@
// that uploaders cannot probe for the existence of projects (e.g. deduce from the error
// message whether a project exists)
return nonResolvableImport(
+ codeOwnerConfigRevision,
branchNameKey,
importType,
+ codeOwnerConfigReference,
codeOwnerConfigFilePath,
String.format("project '%s' not found", keyOfImportedCodeOwnerConfig.project().get()));
}
if (!projectState.get().statePermitsRead()) {
return nonResolvableImport(
+ codeOwnerConfigRevision,
branchNameKey,
importType,
+ codeOwnerConfigReference,
codeOwnerConfigFilePath,
String.format(
"project '%s' has state '%s' that doesn't permit read",
@@ -927,8 +937,10 @@
// that uploaders cannot probe for the existence of branches (e.g. deduce from the error
// message whether a branch exists)
return nonResolvableImport(
+ codeOwnerConfigRevision,
branchNameKey,
importType,
+ codeOwnerConfigReference,
codeOwnerConfigFilePath,
String.format(
"branch '%s' not found in project '%s'",
@@ -943,8 +955,10 @@
if (!codeOwnerBackend.isCodeOwnerConfigFile(
keyOfImportedCodeOwnerConfig.project(), codeOwnerConfigReference.fileName())) {
return nonResolvableImport(
+ codeOwnerConfigRevision,
branchNameKey,
importType,
+ codeOwnerConfigReference,
codeOwnerConfigFilePath,
String.format(
"'%s' is not a code owner config file", codeOwnerConfigReference.filePath()));
@@ -961,8 +975,10 @@
: codeOwnerBackend.getCodeOwnerConfig(keyOfImportedCodeOwnerConfig, revision.get());
if (!importedCodeOwnerConfig.isPresent()) {
return nonResolvableImport(
+ codeOwnerConfigRevision,
branchNameKey,
importType,
+ codeOwnerConfigReference,
codeOwnerConfigFilePath,
String.format(
"'%s' does not exist (project = %s, branch = %s, revision = %s)",
@@ -975,8 +991,10 @@
if (getInvalidConfigCause(codeOwnersInternalServerErrorException).isPresent()) {
// The imported code owner config is non-parseable.
return nonResolvableImport(
+ codeOwnerConfigRevision,
branchNameKey,
importType,
+ codeOwnerConfigReference,
codeOwnerConfigFilePath,
String.format(
"'%s' is not parseable (project = %s, branch = %s)",
@@ -1057,19 +1075,54 @@
}
private Optional<CommitValidationMessage> nonResolvableImport(
+ RevCommit codeOwnerConfigRevision,
BranchNameKey branchNameKey,
CodeOwnerConfigImportType importType,
+ CodeOwnerConfigReference codeOwnerConfigReference,
Path codeOwnerConfigFilePath,
String message) {
- return nonResolvableImport(
- importType,
- codeOwnerConfigFilePath,
- message,
- codeOwnersPluginConfiguration
- .getProjectConfig(branchNameKey.project())
- .rejectNonResolvableImports(branchNameKey.branch())
- ? ValidationMessage.Type.ERROR
- : ValidationMessage.Type.WARNING);
+ ValidationMessage.Type validationMessageType;
+ if (codeOwnerConfigRevision.getParentCount() > 1
+ && !codeOwnerConfigReference.branch().isPresent()
+ && codeOwnerConfigReference.project().isPresent()
+ && !codeOwnerConfigReference.project().get().equals(branchNameKey.project())) {
+ // For merge commits, imports from other projects, that implicitly assume the same branch as
+ // the importing code owner config, should not be rejected if they cannot be resolved. Hence
+ // issues with them are always reported as warnings (rather than errors which would cause a
+ // rejection).
+ //
+ // We do not reject such non-resolvable imports because that can require landing merge
+ // commits in a certain order or even make the landing of merge commits impossible.
+ //
+ // Example 1:
+ // 1. project A adds foo/OWNERS
+ // 2. project B imports A:foo/OWNERS
+ // => If these changes should be merged into another branch (e.g. a release branch) the
+ // validation of the code owner config files only succeeds if the merges are done in the
+ // correct order (1. do merge for project A, 2. do merge for project B). If the merges are
+ // done in the opposite order (1. do merge for project B, 2. do merge for project A) the code
+ // owner config file validation for the merge in project B would fail since A:foo/OWNERS
+ // doesn't exist in the target branch yet.
+ //
+ // Example 2:
+ // 1. project A adds foo/OWNERS
+ // 2. project B imports A:foo/OWNERS
+ // 3. project B adds bar/OWNERS
+ // 4. project A imports B:bar/OWNERS
+ // => If the merge for project A is done first the code owner config file validation would
+ // fail because B:bar/OWNERS doesn't exist yet. If the merge for project B is done first the
+ // code owner config file validation would fail because A:foo/OWNERS doesn't exist yet.
+ validationMessageType = ValidationMessage.Type.WARNING;
+ } else {
+ validationMessageType =
+ codeOwnersPluginConfiguration
+ .getProjectConfig(branchNameKey.project())
+ .rejectNonResolvableImports(branchNameKey.branch())
+ ? ValidationMessage.Type.ERROR
+ : ValidationMessage.Type.WARNING;
+ }
+
+ return nonResolvableImport(importType, codeOwnerConfigFilePath, message, validationMessageType);
}
private Optional<CommitValidationMessage> nonResolvableImport(
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 d2ce328..7f0e207 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -32,6 +33,7 @@
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.Project.NameKey;
+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;
@@ -43,6 +45,7 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestCodeOwnerConfigCreation;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode;
@@ -67,6 +70,7 @@
import java.util.Optional;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
import org.junit.Test;
@@ -1633,6 +1637,208 @@
}
@Test
+ public void
+ forMergeCommitsNonResolvableGlobalImportsFromOtherProjectsAreReportedAsWarningsIfImportsDontSpecifyBranch()
+ throws Exception {
+ testForMergeCommitsThatNonResolvableImportsFromOtherProjectsAreReportedAsWarningsIfImportsDontSpecifyBranch(
+ CodeOwnerConfigImportType.GLOBAL);
+ }
+
+ @Test
+ public void
+ forMergeCommitsNonResolvablePerFileImportsFromOtherProjectsAreReportedAsWarningsIfImportsDontSpecifyBranch()
+ throws Exception {
+ testForMergeCommitsThatNonResolvableImportsFromOtherProjectsAreReportedAsWarningsIfImportsDontSpecifyBranch(
+ CodeOwnerConfigImportType.PER_FILE);
+ }
+
+ private void
+ testForMergeCommitsThatNonResolvableImportsFromOtherProjectsAreReportedAsWarningsIfImportsDontSpecifyBranch(
+ CodeOwnerConfigImportType importType) throws Exception {
+ skipTestIfImportsNotSupportedByCodeOwnersBackend();
+
+ // Create a second project from which we will import a code owner config.
+ Project.NameKey otherProject = projectOperations.newProject().create();
+
+ // Create a target branch for into which we will merge later.
+ String targetBranchName = "target";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = targetBranchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+ branchInput.revision = projectOperations.project(otherProject).getHead("master").name();
+ gApi.projects().name(otherProject.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create the code owner config file in the second project that we will import.
+ CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(otherProject)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ // Create a code owner config that imports the code owner config from the other project, without
+ // specifying the branch for the import (if the branch is not specified the code owner config is
+ // imported from the same branch that contains the importing code owner config).
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportedCodeOwnerConfig)
+ .getFilePath())
+ .setProject(otherProject)
+ .build();
+ TestCodeOwnerConfigCreation.Builder codeOwnerConfigBuilder =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/");
+ switch (importType) {
+ case GLOBAL:
+ codeOwnerConfigBuilder.addImport(codeOwnerConfigReference);
+ break;
+ case PER_FILE:
+ codeOwnerConfigBuilder.addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression("foo")
+ .addImport(codeOwnerConfigReference)
+ .build());
+ break;
+ default:
+ throw new IllegalStateException("unknown import type: " + importType);
+ }
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = codeOwnerConfigBuilder.create();
+ GitUtil.fetch(testRepo, "refs/*:refs/*");
+
+ // Create the merge commit.
+ RevCommit parent1 = projectOperations.project(project).getHead(targetBranchName);
+ RevCommit parent2 = projectOperations.project(project).getHead("master");
+ PushOneCommit m =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "merge",
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(codeOwnerConfigOperations.codeOwnerConfig(keyOfImportingCodeOwnerConfig).get()));
+ m.setParents(ImmutableList.of(parent1, parent2));
+ PushOneCommit.Result r = m.to("refs/for/" + targetBranchName);
+ assertOkWithWarnings(
+ r,
+ "invalid code owner config files",
+ String.format(
+ "invalid %s import in '%s': '%s' does not exist (project = %s, branch = %s,"
+ + " revision = %s)",
+ importType.getType(),
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfImportingCodeOwnerConfig).getFilePath(),
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfImportedCodeOwnerConfig).getFilePath(),
+ otherProject.get(),
+ targetBranchName,
+ parent1.name()));
+ }
+
+ @Test
+ public void
+ forMergeCommitsNonResolvableGlobalImportsFromOtherProjectsAreReportedAsErrorsIfImportsSpecifyBranch()
+ throws Exception {
+ testForMergeCommitsThatNonResolvableImportsFromOtherProjectsAreReportedAsErrorsIfImportsSpecifyBranch(
+ CodeOwnerConfigImportType.GLOBAL);
+ }
+
+ @Test
+ public void
+ forMergeCommitsNonResolvablePerFileImportsFromOtherProjectsAreReportedAsErrorsIfImportsSpecifyBranch()
+ throws Exception {
+ testForMergeCommitsThatNonResolvableImportsFromOtherProjectsAreReportedAsErrorsIfImportsSpecifyBranch(
+ CodeOwnerConfigImportType.PER_FILE);
+ }
+
+ private void
+ testForMergeCommitsThatNonResolvableImportsFromOtherProjectsAreReportedAsErrorsIfImportsSpecifyBranch(
+ CodeOwnerConfigImportType importType) throws Exception {
+ skipTestIfImportsNotSupportedByCodeOwnersBackend();
+
+ // Create a second project from which we will import a non-existing code owner config.
+ Project.NameKey otherProject = projectOperations.newProject().create();
+
+ // Create a target branch for into which we will merge later.
+ String targetBranchName = "target";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = targetBranchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a code owner config that imports a non-existing code owner config from the other
+ // project, with specifying the branch for the import. When this code owner config is merged
+ // into another branch later we expect that it is rejected by the validation.
+ CodeOwnerConfig.Key keyOfNonExistingCodeOwnerConfig =
+ CodeOwnerConfig.Key.create(project, "master", "/foo/");
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfNonExistingCodeOwnerConfig)
+ .getFilePath())
+ .setProject(otherProject)
+ .setBranch("master")
+ .build();
+ TestCodeOwnerConfigCreation.Builder codeOwnerConfigBuilder =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/");
+ switch (importType) {
+ case GLOBAL:
+ codeOwnerConfigBuilder.addImport(codeOwnerConfigReference);
+ break;
+ case PER_FILE:
+ codeOwnerConfigBuilder.addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression("foo")
+ .addImport(codeOwnerConfigReference)
+ .build());
+ break;
+ default:
+ throw new IllegalStateException("unknown import type: " + importType);
+ }
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = codeOwnerConfigBuilder.create();
+ GitUtil.fetch(testRepo, "refs/*:refs/*");
+
+ // Create the merge commit.
+ RevCommit parent1 = projectOperations.project(project).getHead(targetBranchName);
+ RevCommit parent2 = projectOperations.project(project).getHead("master");
+ PushOneCommit m =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "merge",
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(codeOwnerConfigOperations.codeOwnerConfig(keyOfImportingCodeOwnerConfig).get()));
+ m.setParents(ImmutableList.of(parent1, parent2));
+ PushOneCommit.Result r = m.to("refs/for/" + targetBranchName);
+ assertErrorWithMessages(
+ r,
+ "invalid code owner config files",
+ String.format(
+ "invalid %s import in '%s': '%s' does not exist (project = %s, branch = master,"
+ + " revision = %s)",
+ importType.getType(),
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfImportingCodeOwnerConfig).getFilePath(),
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfNonExistingCodeOwnerConfig)
+ .getFilePath(),
+ otherProject.get(),
+ parent1.name()));
+ }
+
+ @Test
public void cannotUploadConfigWithGlobalImportOfNonParseableCodeOwnerConfig() throws Exception {
testUploadConfigWithImportOfNonParseableCodeOwnerConfig(CodeOwnerConfigImportType.GLOBAL);
}
