Merge "Return code-owners~skip-validation option only if OWNERS files are touched"
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
index 2917a90..316da14 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
@@ -20,13 +20,19 @@
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.ChangedFiles;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
import com.google.gerrit.server.PluginPushOption;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.io.IOException;
+import java.nio.file.Path;
/** Push option that allows to skip the code owner config validation. */
@Singleton
@@ -38,17 +44,20 @@
private final PermissionBackend permissionBackend;
private final SkipCodeOwnerConfigValidationCapability skipCodeOwnerConfigValidationCapability;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
+ private final ChangedFiles changedFiles;
@Inject
SkipCodeOwnerConfigValidationPushOption(
@PluginName String pluginName,
PermissionBackend permissionBackend,
SkipCodeOwnerConfigValidationCapability skipCodeOwnerConfigValidationCapability,
- CodeOwnersPluginConfiguration codeOwnersPluginConfiguration) {
+ CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
+ ChangedFiles changedFiles) {
this.pluginName = pluginName;
this.permissionBackend = permissionBackend;
this.skipCodeOwnerConfigValidationCapability = skipCodeOwnerConfigValidationCapability;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
+ this.changedFiles = changedFiles;
}
@Override
@@ -66,7 +75,39 @@
return !codeOwnersPluginConfiguration
.getProjectConfig(changeNotes.getProjectName())
.isDisabled(changeNotes.getChange().getDest().branch())
- && canSkipCodeOwnerConfigValidation();
+ && canSkipCodeOwnerConfigValidation()
+ && hasModifiedCodeOwnerConfigFiles(changeNotes);
+ }
+
+ private boolean hasModifiedCodeOwnerConfigFiles(ChangeNotes changeNotes) {
+ CodeOwnerBackend codeOwnerBackend =
+ codeOwnersPluginConfiguration
+ .getProjectConfig(changeNotes.getProjectName())
+ .getBackend(changeNotes.getChange().getDest().branch());
+
+ // For merge commits, always do the comparison against the destination branch
+ // (MergeCommitStrategy.ALL_CHANGED_FILES) as this is what CodeOwnerConfigValidator does.
+ try {
+ return changedFiles
+ .getFromDiffCache(
+ changeNotes.getProjectName(),
+ changeNotes.getCurrentPatchSet().commitId(),
+ MergeCommitStrategy.ALL_CHANGED_FILES)
+ .stream()
+ // filter out deletions (files without new path)
+ .filter(changedFile -> changedFile.newPath().isPresent())
+ .anyMatch(
+ changedFile ->
+ codeOwnerBackend.isCodeOwnerConfigFile(
+ changeNotes.getProjectName(),
+ Path.of(changedFile.newPath().get().toString()).getFileName().toString()));
+ } catch (IOException | DiffNotAvailableException e) {
+ throw newInternalServerError(
+ String.format(
+ "Failed to check changed files of change %s in project %s",
+ changeNotes.getChangeId(), changeNotes.getProjectName()),
+ e);
+ }
}
/**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/SkipCodeOwnerConfigValidationPushOptionIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/SkipCodeOwnerConfigValidationPushOptionIT.java
index 6c74375..dd2cf9d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/SkipCodeOwnerConfigValidationPushOptionIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/SkipCodeOwnerConfigValidationPushOptionIT.java
@@ -21,30 +21,56 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
+import com.google.gerrit.acceptance.testsuite.change.TestChangeCreation;
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.Change;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.common.ValidationOptionInfo;
import com.google.gerrit.extensions.common.ValidationOptionInfos;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
+import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
+import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
+import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersCodeOwnerConfigParser;
+import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
+import com.google.gerrit.plugins.codeowners.backend.proto.ProtoCodeOwnerConfigParser;
import com.google.gerrit.plugins.codeowners.validation.SkipCodeOwnerConfigValidationCapability;
import com.google.gerrit.plugins.codeowners.validation.SkipCodeOwnerConfigValidationPushOption;
import com.google.inject.Inject;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Before;
import org.junit.Test;
public class SkipCodeOwnerConfigValidationPushOptionIT extends AbstractCodeOwnersIT {
+ private static final ObjectId TEST_REVISION =
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+
@Inject private ChangeOperations changeOperations;
@Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
+ private FindOwnersCodeOwnerConfigParser findOwnersCodeOwnerConfigParser;
+ private ProtoCodeOwnerConfigParser protoCodeOwnerConfigParser;
+
+ @Before
+ public void setUpCodeOwnersPlugin() throws Exception {
+ backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class);
+ findOwnersCodeOwnerConfigParser =
+ plugin.getSysInjector().getInstance(FindOwnersCodeOwnerConfigParser.class);
+ protoCodeOwnerConfigParser =
+ plugin.getSysInjector().getInstance(ProtoCodeOwnerConfigParser.class);
+ }
+
@Test
public void getCodeOwnersSkipOptionAsAdmin() throws Exception {
// Use the admin user that has the SkipCodeOwnerConfigValidationCapability global capability
// implicitly assigned.
requestScopeOperations.setApiUser(admin.id());
- Change.Id changeId = changeOperations.newChange().project(project).create();
-
+ Change.Id changeId = createChangeWithCodeOwnerConfigFile(project);
ValidationOptionInfos validationOptionsInfos =
gApi.changes().id(project.get(), changeId.get()).getValidationOptions();
assertThat(validationOptionsInfos.validationOptions)
@@ -66,9 +92,7 @@
.update();
requestScopeOperations.setApiUser(user.id());
-
- Change.Id changeId = changeOperations.newChange().project(project).create();
-
+ Change.Id changeId = createChangeWithCodeOwnerConfigFile(project);
ValidationOptionInfos validationOptionsInfos =
gApi.changes().id(project.get(), changeId.get()).getValidationOptions();
assertThat(validationOptionsInfos.validationOptions)
@@ -86,7 +110,7 @@
// capability.
requestScopeOperations.setApiUser(user.id());
- Change.Id changeId = changeOperations.newChange().project(project).create();
+ Change.Id changeId = createChangeWithCodeOwnerConfigFile(project);
ValidationOptionInfos validationOptionsInfos =
gApi.changes().id(project.get(), changeId.get()).getValidationOptions();
assertThat(validationOptionsInfos.validationOptions).isEmpty();
@@ -96,7 +120,7 @@
@Test
public void codeOwnersSkipOptionIsOmittedIfCodeOwnersFunctionalityIsDisabledForProject()
throws Exception {
- Change.Id changeId = changeOperations.newChange().project(project).create();
+ Change.Id changeId = createChangeWithCodeOwnerConfigFile(project);
ValidationOptionInfos validationOptionsInfos =
gApi.changes().id(project.get(), changeId.get()).getValidationOptions();
assertThat(validationOptionsInfos.validationOptions).isEmpty();
@@ -106,9 +130,57 @@
@Test
public void codeOwnersSkipOptionIsOmittedIfCodeOwnersFunctionalityIsDisabledForBranch()
throws Exception {
- Change.Id changeId = changeOperations.newChange().project(project).branch("master").create();
+ Change.Id changeId = createChangeWithCodeOwnerConfigFile(project, "master");
ValidationOptionInfos validationOptionsInfos =
gApi.changes().id(project.get(), changeId.get()).getValidationOptions();
assertThat(validationOptionsInfos.validationOptions).isEmpty();
}
+
+ @Test
+ public void codeOwnersSkipOptionIsOmittedIfChangeDoesNotTouchCodeOwnerConfigs() throws Exception {
+ requestScopeOperations.setApiUser(admin.id());
+ Change.Id changeId = changeOperations.newChange().project(project).create();
+ ValidationOptionInfos validationOptionsInfos =
+ gApi.changes().id(project.get(), changeId.get()).getValidationOptions();
+ assertThat(validationOptionsInfos.validationOptions).isEmpty();
+ }
+
+ private Change.Id createChangeWithCodeOwnerConfigFile(Project.NameKey project) throws Exception {
+ return createChangeWithCodeOwnerConfigFile(project, /* branch= */ null);
+ }
+
+ private Change.Id createChangeWithCodeOwnerConfigFile(
+ Project.NameKey project, @Nullable String branch) throws Exception {
+ CodeOwnerConfig.Key codeOwnerConfigKey = CodeOwnerConfig.Key.create(project, "master", "/");
+ String codeOwnerConfigFile =
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath();
+
+ TestChangeCreation.Builder testChangeCreationBuilder =
+ changeOperations.newChange().project(project);
+ if (branch != null) {
+ testChangeCreationBuilder.branch(branch);
+ }
+
+ return testChangeCreationBuilder
+ .file(codeOwnerConfigFile)
+ .content(
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addCodeOwnerSet(CodeOwnerSet.createWithoutPathExpressions(admin.email()))
+ .build()))
+ .create();
+ }
+
+ private String format(CodeOwnerConfig codeOwnerConfig) throws Exception {
+ if (backendConfig.getDefaultBackend() instanceof FindOwnersBackend) {
+ return findOwnersCodeOwnerConfigParser.formatAsString(codeOwnerConfig);
+ } else if (backendConfig.getDefaultBackend() instanceof ProtoBackend) {
+ return protoCodeOwnerConfigParser.formatAsString(codeOwnerConfig);
+ }
+
+ throw new IllegalStateException(
+ String.format(
+ "unknown code owner backend: %s",
+ backendConfig.getDefaultBackend().getClass().getName()));
+ }
}
