Merge changes Iafb16808,I2feeb31f,I1ef01100,I51873396,I0a3d985a, ...
* changes:
Prefer explicitly mentioned code owners
Test getting code owners when all users are global code owners
Fix getAllUsersAsCodeOwners_allVisible test
JgitPathTest: Remove unneeded import
JGitPathTest: Test toString/equals/hashCode methods
Test adding config with ALL import of config that is added in same commit
Include revision into error message when rejecting invalid import
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 71cb59f..3dd3ab9 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -370,13 +370,17 @@
logger.atFine().log("filling up with random users");
codeOwners.addAll(
filterCodeOwners(
- rsrc,
- // ask for 2 times the number of users that we need so that we still have enough
- // suggestions when some users are removed by the filterCodeOwners call or if the
- // returned users were already present in codeOwners
- getRandomVisibleUsers(2 * limit - codeOwners.size())
- .map(CodeOwner::create)
- .collect(toImmutableSet())));
+ rsrc,
+ // ask for 2 times the number of users that we need so that we still have enough
+ // suggestions when some users are removed by the filterCodeOwners call or if the
+ // returned users were already present in codeOwners
+ getRandomVisibleUsers(2 * limit - codeOwners.size())
+ .map(CodeOwner::create)
+ .collect(toImmutableSet()))
+ .stream()
+ .filter(codeOwner -> !codeOwners.contains(codeOwner))
+ .limit(limit - codeOwners.size())
+ .collect(toImmutableSet()));
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 17235cd..9458289 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -872,10 +872,11 @@
importType,
codeOwnerConfigFilePath,
String.format(
- "'%s' does not exist (project = %s, branch = %s)",
+ "'%s' does not exist (project = %s, branch = %s, revision = %s)",
codeOwnerConfigReference.filePath(),
keyOfImportedCodeOwnerConfig.branchNameKey().project().get(),
- keyOfImportedCodeOwnerConfig.branchNameKey().shortName()));
+ keyOfImportedCodeOwnerConfig.branchNameKey().shortName(),
+ revision.get().name()));
}
} catch (StorageException storageException) {
if (getInvalidConfigCause(storageException).isPresent()) {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
index 4dbac76..6e14d21 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -642,6 +642,30 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void getAllUsersAsGlobalCodeOwners() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id(), user2.id(), admin.id());
+
+ // Query code owners with a limit.
+ requestScopeOperations.setApiUser(user.id());
+ codeOwnerInfos = queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ assertThat(codeOwnerInfos).hasSize(2);
+ assertThatList(codeOwnerInfos)
+ .element(0)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id(), admin.id());
+ assertThatList(codeOwnerInfos)
+ .element(1)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id(), admin.id());
+ }
+
+ @Test
@GerritConfig(
name = "plugin.code-owners.globalCodeOwner",
values = {"global.owner1@example.com", "global.owner2@example.com"})
@@ -861,6 +885,7 @@
.containsExactly(user.id(), user2.id(), admin.id());
// Query code owners with a limit.
+ requestScopeOperations.setApiUser(user.id());
codeOwnerInfos = queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
assertThat(codeOwnerInfos).hasSize(2);
assertThatList(codeOwnerInfos)
@@ -1016,6 +1041,33 @@
}
@Test
+ public void getAllUsersAsCodeOwners_explicitlyMentionedCodeOwnersArePreferred() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // Add a code owner config that assigns code ownership to user2 and all users.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(user2.email())
+ .addCodeOwnerEmail("*")
+ .create();
+
+ // Query code owners with limits, "*" is resolved to random users, but user2 should always be
+ // included since this user is set explicitly as code owner
+ List<CodeOwnerInfo> codeOwnerInfos =
+ queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ assertThat(codeOwnerInfos).hasSize(2);
+ assertThatList(codeOwnerInfos).comparingElementsUsing(hasAccountId()).contains(user2.id());
+
+ codeOwnerInfos = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ assertThatList(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user2.id());
+ }
+
+ @Test
public void getCodeOwnersProvidingASeedMakesSortOrderStableAcrocssRequests() throws Exception {
TestAccount user2 = accountCreator.user2();
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 fe9bdc1..314747c 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -289,8 +289,9 @@
}
@Test
- public void canUploadConfigWithoutIssues_withImportOfConfigThatIsAddedInSameCommit()
- throws Exception {
+ public void
+ canUploadConfigWithImportOfConfigThatIsAddedInSameCommit_importModeGlobalCodeOwnersOnly()
+ throws Exception {
// imports are not supported for the proto backend
assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
@@ -302,7 +303,6 @@
CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
codeOwnerConfigOperations.codeOwnerConfig(keyOfImportedCodeOwnerConfig).getFilePath());
- // Create a code owner config with import and without issues.
PushOneCommit.Result r =
createChange(
"Add code owners",
@@ -330,6 +330,42 @@
}
@Test
+ public void canUploadConfigWithImportOfConfigThatIsAddedInSameCommit_importModeAll()
+ throws Exception {
+ // imports are not supported for the proto backend
+ assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
+
+ CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/");
+ CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig = createCodeOwnerConfigKey("/foo/");
+
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.ALL,
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfImportedCodeOwnerConfig).getFilePath());
+
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ ImmutableMap.of(
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addImport(codeOwnerConfigReference)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder().addCodeOwnerEmail(admin.email()).build())
+ .build()),
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportedCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(keyOfImportedCodeOwnerConfig, TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder().addCodeOwnerEmail(user.email()).build())
+ .build())));
+ assertOkWithHints(r, "code owner config files validated, no issues found");
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.backend", value = "non-existing-backend")
public void canUploadNonParseableConfigIfCodeOwnersPluginConfigurationIsInvalid()
throws Exception {
@@ -1355,13 +1391,15 @@
r,
"invalid code owner config files",
String.format(
- "invalid %s import in '%s': '%s' does not exist (project = %s, branch = master)",
+ "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(),
- project.get()));
+ project.get(),
+ r.getCommit().name()));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/util/JgitPathTest.java b/javatests/com/google/gerrit/plugins/codeowners/util/JgitPathTest.java
index 25f4035..4b73459 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/util/JgitPathTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/util/JgitPathTest.java
@@ -19,12 +19,11 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
-import com.google.gerrit.plugins.codeowners.util.JgitPath;
import java.nio.file.Path;
import java.nio.file.Paths;
import org.junit.Test;
-/** Tests for {@link JgitPath}. */
+/** Tests for {@link com.google.gerrit.plugins.codeowners.util.JgitPath}. */
public class JgitPathTest extends AbstractCodeOwnersTest {
@Test
public void getJgitPathOfStringPath() throws Exception {
@@ -57,4 +56,29 @@
assertThat(JgitPath.of("foo/bar/OWNERS").getAsAbsolutePath())
.isEqualTo(Paths.get("/foo/bar/OWNERS"));
}
+
+ @Test
+ public void testToString() throws Exception {
+ String path = "foo/bar/baz.md";
+ assertThat(JgitPath.of(path).toString()).isEqualTo(path);
+ assertThat(JgitPath.of("/" + path).toString()).isEqualTo(path);
+ }
+
+ @Test
+ public void testEquals() throws Exception {
+ String path = "foo/bar/baz.md";
+ assertThat(JgitPath.of(path)).isEqualTo(JgitPath.of(path));
+ assertThat(JgitPath.of("/" + path)).isEqualTo(JgitPath.of(path));
+ assertThat(JgitPath.of("/" + path)).isNotEqualTo(JgitPath.of("foo/bar/baz.txt"));
+ assertThat(JgitPath.of("/" + path)).isNotEqualTo(new Object());
+ }
+
+ @Test
+ public void testHashCode() throws Exception {
+ String path = "foo/bar/baz.md";
+ assertThat(JgitPath.of(path).hashCode()).isEqualTo(JgitPath.of(path).hashCode());
+ assertThat(JgitPath.of("/" + path).hashCode()).isEqualTo(JgitPath.of(path).hashCode());
+ assertThat(JgitPath.of("/" + path).hashCode())
+ .isNotEqualTo(JgitPath.of("foo/bar/baz.txt").hashCode());
+ }
}
