Disable OWNERS validation that cannot be satisfied for merge commits
When OWNERS files are modified the validation verifies that newly added
imports can be resolved. If an OWNERS file imports an OWNERS file from
another project without specifying the source branch it assumed that the
OWNERS file should be imported from the same branch in which the
importing OWNERS file is contained. E.g. if an OWNERS file in project A
in branch master contains "file:B:foo/OWNERS" (import 'foo/OWNERS' from
project B) 'foo/OWNERS' is imported from the master branch in project B.
If that OWNERS file in project A gets merged into another branch, e.g.
the 'release' branch, then the same import "file:B:foo/OWNERS" imports
'foo/OWNERS' from the release branch in project B. If the validation
rejects such imports if they are not resolvable (e.g. the imported
OWNERS file doesn't exist) this can enforce landing merges in a certain
order or even make landing merges 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.
To make the landing of merges possible in this case, we now report
non-resolvable imports that are newly added in merge commits as warnings
if the import is done from another branch and the branch is not
specified but assumed to be the same as the branch that contains the
importing OWNERS file. In contrast to errors, warnings do not lead to a
rejection of the commit, but are purely informational.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Iadbe4262dc2791393810dc85da6d5dd70d21ac98
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);
}
