Add inheritance from parent project
Allow definitiion of the OWNERS file in the parent project.
The OWNERS file in the root of the project and the one defined at
project level will inherit from it if the "inherited" flag is
enabled.
Change-Id: I50c834660126715b521928e558e83f45dd583fc0
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
index 4fac8ec..35828f4 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
@@ -45,6 +45,7 @@
import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
@@ -71,6 +72,7 @@
private final GerritApi api;
private final PatchListCache patchListCache;
+ private final ProjectCache projectCache;
private final GitRepositoryManager repositoryManager;
private final Accounts accounts;
private final ReviewerManager reviewerManager;
@@ -87,6 +89,7 @@
public GitRefListener(
GerritApi api,
PatchListCache patchListCache,
+ ProjectCache projectCache,
GitRepositoryManager repositoryManager,
Accounts accounts,
ReviewerManager reviewerManager,
@@ -96,6 +99,7 @@
AutoassignConfig cfg) {
this.api = api;
this.patchListCache = patchListCache;
+ this.projectCache = projectCache;
this.repositoryManager = repositoryManager;
this.accounts = accounts;
this.reviewerManager = reviewerManager;
@@ -207,12 +211,19 @@
try {
ChangeApi cApi = changes.id(cId.get());
ChangeInfo change = cApi.get();
+ Optional<Project.NameKey> maybeParentProjectNameKey =
+ projectCache
+ .get(Project.NameKey.parse(change.project))
+ .map(p -> p.getProject().getParent());
+
PatchList patchList = getPatchList(repository, event, change);
if (patchList != null) {
PathOwners owners =
new PathOwners(
accounts,
+ repositoryManager,
repository,
+ maybeParentProjectNameKey,
cfg.isBranchDisabled(change.branch) ? Optional.empty() : Optional.of(change.branch),
patchList,
cfg.expandGroups());
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
index e460136..bc093be 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
@@ -22,6 +22,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -42,6 +43,7 @@
public GitRefListenerTest(
GerritApi api,
PatchListCache patchListCache,
+ ProjectCache projectCache,
GitRepositoryManager repositoryManager,
Accounts accounts,
ReviewerManager reviewerManager,
@@ -52,6 +54,7 @@
super(
api,
patchListCache,
+ projectCache,
repositoryManager,
accounts,
reviewerManager,
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
index 9bdf9b8..d37a086 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -27,7 +27,9 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Account.Id;
import com.google.gerrit.entities.Patch;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListEntry;
import java.io.IOException;
@@ -54,12 +56,16 @@
private final Repository repository;
+ private final Optional<Project.NameKey> maybeParentRepo;
+
private final PatchList patchList;
private final ConfigurationParser parser;
private final Accounts accounts;
+ private final GitRepositoryManager repositoryManager;
+
private Map<String, Matcher> matchers;
private Map<String, Set<Id>> fileOwners;
@@ -70,11 +76,15 @@
public PathOwners(
Accounts accounts,
+ GitRepositoryManager repositoryManager,
Repository repository,
+ Optional<Project.NameKey> maybeParentRepo,
Optional<String> branchWhenEnabled,
PatchList patchList,
boolean expandGroups) {
+ this.repositoryManager = repositoryManager;
this.repository = repository;
+ this.maybeParentRepo = maybeParentRepo;
this.patchList = patchList;
this.parser = new ConfigurationParser(accounts);
this.accounts = accounts;
@@ -130,41 +140,21 @@
private OwnersMap fetchOwners(String branch) {
OwnersMap ownersMap = new OwnersMap();
try {
- String rootPath = "OWNERS";
-
- PathOwnersEntry projectEntry =
- getOwnersConfig(rootPath, RefNames.REFS_CONFIG)
- .map(
- conf ->
- new PathOwnersEntry(
- rootPath,
- conf,
- accounts,
- Collections.emptySet(),
- Collections.emptySet(),
- Collections.emptySet(),
- Collections.emptySet()))
- .orElse(new PathOwnersEntry());
-
- PathOwnersEntry rootEntry =
- getOwnersConfig(rootPath, branch)
- .map(
- conf ->
- new PathOwnersEntry(
- rootPath,
- conf,
- accounts,
- Collections.emptySet(),
- Collections.emptySet(),
- Collections.emptySet(),
- Collections.emptySet()))
- .orElse(new PathOwnersEntry());
+ // Using a `map` would have needed a try/catch inside the lamba, resulting in more code
+ Optional<PathOwnersEntry> maybeParentPathOwnersEntry =
+ maybeParentRepo.isPresent()
+ ? Optional.of(getPathOwnersEntry(maybeParentRepo.get(), RefNames.REFS_CONFIG))
+ : Optional.empty();
+ PathOwnersEntry projectEntry = getPathOwnersEntry(repository, RefNames.REFS_CONFIG);
+ PathOwnersEntry rootEntry = getPathOwnersEntry(repository, branch);
Set<String> modifiedPaths = getModifiedPaths();
Map<String, PathOwnersEntry> entries = new HashMap<>();
PathOwnersEntry currentEntry = null;
for (String path : modifiedPaths) {
- currentEntry = resolvePathEntry(path, branch, projectEntry, rootEntry, entries);
+ currentEntry =
+ resolvePathEntry(
+ path, branch, projectEntry, maybeParentPathOwnersEntry, rootEntry, entries);
// add owners and reviewers to file for matcher predicates
ownersMap.addFileOwners(path, currentEntry.getOwners());
@@ -199,6 +189,29 @@
}
}
+ private PathOwnersEntry getPathOwnersEntry(Project.NameKey projectName, String branch)
+ throws IOException {
+ try (Repository repo = repositoryManager.openRepository(projectName)) {
+ return getPathOwnersEntry(repo, branch);
+ }
+ }
+
+ private PathOwnersEntry getPathOwnersEntry(Repository repo, String branch) throws IOException {
+ String rootPath = "OWNERS";
+ return getOwnersConfig(repo, rootPath, branch)
+ .map(
+ conf ->
+ new PathOwnersEntry(
+ rootPath,
+ conf,
+ accounts,
+ Collections.emptySet(),
+ Collections.emptySet(),
+ Collections.emptySet(),
+ Collections.emptySet()))
+ .orElse(new PathOwnersEntry());
+ }
+
private void processMatcherPerPath(
Map<String, Matcher> fullMatchers,
HashMap<String, Matcher> newMatchers,
@@ -219,6 +232,7 @@
String path,
String branch,
PathOwnersEntry projectEntry,
+ Optional<PathOwnersEntry> maybeParentPathOwnersEntry,
PathOwnersEntry rootEntry,
Map<String, PathOwnersEntry> entries)
throws IOException {
@@ -226,18 +240,12 @@
PathOwnersEntry currentEntry = rootEntry;
StringBuilder builder = new StringBuilder();
- if (rootEntry.isInherited()) {
- for (Matcher matcher : projectEntry.getMatchers().values()) {
- if (!currentEntry.hasMatcher(matcher.getPath())) {
- currentEntry.addMatcher(matcher);
- }
- }
- if (currentEntry.getOwners().isEmpty()) {
- currentEntry.setOwners(projectEntry.getOwners());
- }
- if (currentEntry.getOwnersPath() == null) {
- currentEntry.setOwnersPath(projectEntry.getOwnersPath());
- }
+ // Inherit from Project if OWNER in root enables inheritance
+ calculateCurrentEntry(rootEntry, projectEntry, currentEntry);
+
+ // Inherit from Parent Project if OWNER in Project enables inheritance
+ if (maybeParentPathOwnersEntry.isPresent()) {
+ calculateCurrentEntry(projectEntry, maybeParentPathOwnersEntry.get(), currentEntry);
}
// Iterate through the parent paths, not including the file name
@@ -252,7 +260,7 @@
currentEntry = entries.get(partial);
} else {
String ownersPath = partial + "OWNERS";
- Optional<OwnersConfig> conf = getOwnersConfig(ownersPath, branch);
+ Optional<OwnersConfig> conf = getOwnersConfig(repository, ownersPath, branch);
final Set<Id> owners = currentEntry.getOwners();
final Set<Id> reviewers = currentEntry.getReviewers();
Collection<Matcher> inheritedMatchers = currentEntry.getMatchers().values();
@@ -275,6 +283,23 @@
return currentEntry;
}
+ private void calculateCurrentEntry(
+ PathOwnersEntry rootEntry, PathOwnersEntry projectEntry, PathOwnersEntry currentEntry) {
+ if (rootEntry.isInherited()) {
+ for (Matcher matcher : projectEntry.getMatchers().values()) {
+ if (!currentEntry.hasMatcher(matcher.getPath())) {
+ currentEntry.addMatcher(matcher);
+ }
+ }
+ if (currentEntry.getOwners().isEmpty()) {
+ currentEntry.setOwners(projectEntry.getOwners());
+ }
+ if (currentEntry.getOwnersPath() == null) {
+ currentEntry.setOwnersPath(projectEntry.getOwnersPath());
+ }
+ }
+ }
+
/**
* Parses the patch list for any paths that were modified.
*
@@ -305,9 +330,8 @@
* @return config or null if it doesn't exist
* @throws IOException
*/
- private Optional<OwnersConfig> getOwnersConfig(String ownersPath, String branch)
+ private Optional<OwnersConfig> getOwnersConfig(Repository repo, String ownersPath, String branch)
throws IOException {
- return getBlobAsBytes(repository, branch, ownersPath)
- .flatMap(bytes -> parser.getOwnersConfig(bytes));
+ return getBlobAsBytes(repo, branch, ownersPath).flatMap(parser::getOwnersConfig);
}
}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
index 9b502ea..0426488 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
@@ -20,6 +20,7 @@
import com.google.common.base.Charsets;
import com.google.gerrit.entities.Patch;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListEntry;
import java.io.IOException;
@@ -33,7 +34,9 @@
@Ignore
public abstract class Config {
+ protected GitRepositoryManager repositoryManager;
protected Repository repository;
+ protected Repository parentRepository;
protected PatchList patchList;
protected ConfigurationParser parser;
protected TestAccounts accounts = new TestAccounts();
@@ -42,7 +45,9 @@
public void setup() throws Exception {
PowerMock.mockStatic(JgitWrapper.class);
+ repositoryManager = PowerMock.createMock(GitRepositoryManager.class);
repository = PowerMock.createMock(Repository.class);
+ parentRepository = PowerMock.createMock(Repository.class);
parser = new ConfigurationParser(accounts);
}
@@ -54,6 +59,18 @@
.anyTimes();
}
+ void expectConfig(String path, String branch, String config) throws IOException {
+ expect(JgitWrapper.getBlobAsBytes(anyObject(Repository.class), eq(branch), eq(path)))
+ .andReturn(Optional.of(config.getBytes()))
+ .anyTimes();
+ }
+
+ void expectConfig(String path, String branch, Repository repo, String config) throws IOException {
+ expect(JgitWrapper.getBlobAsBytes(eq(repo), eq(branch), eq(path)))
+ .andReturn(Optional.of(config.getBytes()))
+ .anyTimes();
+ }
+
void expectNoConfig(String path) throws IOException {
expect(
JgitWrapper.getBlobAsBytes(
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
index 2dea5ea..07a83d1 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -14,12 +14,20 @@
package com.googlesource.gerrit.owners.common;
+import static com.googlesource.gerrit.owners.common.MatcherConfig.suffixMatcher;
+import static org.easymock.EasyMock.eq;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.expectLastCall;
import static org.junit.Assert.*;
import static org.powermock.api.easymock.PowerMock.replayAll;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
import java.io.IOException;
import java.util.Arrays;
+import java.util.Collections;
+import java.util.Map;
import java.util.Optional;
import java.util.Set;
import org.junit.Before;
@@ -38,6 +46,8 @@
private static final boolean EXPAND_GROUPS = true;
private static final boolean DO_NOT_EXPAND_GROUPS = false;
public static final String CLASSIC_FILE_TXT = "classic/file.txt";
+ public static final Project.NameKey parentRepositoryNameKey =
+ Project.NameKey.parse("parentRepository");
@Override
@Before
@@ -49,7 +59,15 @@
public void testClassic() throws Exception {
mockOwners(USER_A_EMAIL_COM, USER_B_EMAIL_COM);
- PathOwners owners = new PathOwners(accounts, repository, branch, patchList, EXPAND_GROUPS);
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.empty(),
+ branch,
+ patchList,
+ EXPAND_GROUPS);
Set<Account.Id> ownersSet = owners.get().get(CLASSIC_OWNERS);
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A_ID));
@@ -62,7 +80,14 @@
mockOwners(USER_A_EMAIL_COM, USER_B_EMAIL_COM);
PathOwners owners =
- new PathOwners(accounts, repository, branch, patchList, DO_NOT_EXPAND_GROUPS);
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.empty(),
+ branch,
+ patchList,
+ DO_NOT_EXPAND_GROUPS);
Set<String> ownersSet = owners.getFileGroupOwners().get(CLASSIC_FILE_TXT);
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A));
@@ -75,7 +100,14 @@
mockOwners(USER_A_EMAIL_COM);
PathOwners owners =
- new PathOwners(accounts, repository, Optional.empty(), patchList, EXPAND_GROUPS);
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.empty(),
+ Optional.empty(),
+ patchList,
+ EXPAND_GROUPS);
Set<Account.Id> ownersSet = owners.get().get(CLASSIC_OWNERS);
assertEquals(0, ownersSet.size());
}
@@ -88,7 +120,15 @@
creatingPatchList(Arrays.asList("classic/file.txt"));
replayAll();
- PathOwners owners2 = new PathOwners(accounts, repository, branch, patchList, EXPAND_GROUPS);
+ PathOwners owners2 =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.empty(),
+ branch,
+ patchList,
+ EXPAND_GROUPS);
Set<Account.Id> ownersSet2 = owners2.get().get(CLASSIC_OWNERS);
// in this case we are inheriting the acct3 from /OWNERS
@@ -99,6 +139,80 @@
}
@Test
+ public void testRootInheritFromProject() throws Exception {
+ expectConfig("OWNERS", "master", createConfig(true, owners()));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ createConfig(true, owners(), suffixMatcher(".sql", USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
+
+ String fileName = "file.sql";
+ creatingPatchList(Collections.singletonList(fileName));
+ replayAll();
+
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.empty(),
+ branch,
+ patchList,
+ EXPAND_GROUPS);
+
+ Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
+ assertEquals(1, fileOwners.size());
+
+ Set<Account.Id> ownersSet = fileOwners.get(fileName);
+ assertEquals(2, ownersSet.size());
+ assertTrue(ownersSet.contains(USER_A_ID));
+ assertTrue(ownersSet.contains(USER_B_ID));
+ }
+
+ @Test
+ public void testProjectInheritFromParentProject() throws Exception {
+ expectConfig("OWNERS", "master", createConfig(true, owners()));
+ expectConfig("OWNERS", RefNames.REFS_CONFIG, repository, createConfig(true, owners()));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ parentRepository,
+ createConfig(true, owners(), suffixMatcher(".sql", USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
+
+ String fileName = "file.sql";
+ creatingPatchList(Collections.singletonList(fileName));
+
+ mockParentRepository();
+ replayAll();
+
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.of(parentRepositoryNameKey),
+ branch,
+ patchList,
+ EXPAND_GROUPS);
+
+ Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
+ assertEquals(fileOwners.size(), 1);
+
+ Set<Account.Id> ownersSet = fileOwners.get(fileName);
+ assertEquals(2, ownersSet.size());
+ assertTrue(ownersSet.contains(USER_A_ID));
+ assertTrue(ownersSet.contains(USER_B_ID));
+ }
+
+ private void mockParentRepository() throws IOException {
+ expect(repositoryManager.openRepository(eq(parentRepositoryNameKey)))
+ .andReturn(parentRepository)
+ .anyTimes();
+ parentRepository.close();
+ expectLastCall();
+ }
+
+ @Test
public void testClassicWithInheritanceAndDeepNesting() throws Exception {
expectConfig("OWNERS", createConfig(true, owners(USER_C_EMAIL_COM)));
expectConfig("dir/OWNERS", createConfig(true, owners(USER_B_EMAIL_COM)));
@@ -107,7 +221,15 @@
creatingPatchList(Arrays.asList("dir/subdir/file.txt"));
replayAll();
- PathOwners owners = new PathOwners(accounts, repository, branch, patchList, EXPAND_GROUPS);
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.empty(),
+ branch,
+ patchList,
+ EXPAND_GROUPS);
Set<Account.Id> ownersSet = owners.get().get("dir/subdir/OWNERS");
assertEquals(3, ownersSet.size());
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
index 454ad22..aeffb90 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
@@ -151,7 +151,15 @@
replayAll();
// function under test
- PathOwners owners = new PathOwners(accounts, repository, branch, patchList, EXPAND_GROUPS);
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.empty(),
+ branch,
+ patchList,
+ EXPAND_GROUPS);
// assertions on classic owners
Set<Account.Id> ownersSet = owners.get().get("project/OWNERS");
@@ -247,7 +255,15 @@
creatingPatch("project/file.sql", "another.txt");
replayAll();
- PathOwners owners = new PathOwners(accounts, repository, branch, patchList, EXPAND_GROUPS);
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Optional.empty(),
+ branch,
+ patchList,
+ EXPAND_GROUPS);
Set<String> ownedFiles = owners.getFileOwners().keySet();
assertThat(ownedFiles).containsExactly("project/file.sql");
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
index 4d51472..72f2cf4 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
@@ -16,7 +16,10 @@
package com.googlesource.gerrit.owners;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.rules.StoredValue;
import com.google.gerrit.server.rules.StoredValues;
import com.googlecode.prolog_cafe.lang.Prolog;
@@ -45,10 +48,18 @@
protected PathOwners createValue(Prolog engine) {
PatchList patchList = StoredValues.PATCH_LIST.get(engine);
Repository repository = StoredValues.REPOSITORY.get(engine);
+ ProjectState projectState = StoredValues.PROJECT_STATE.get(engine);
+ GitRepositoryManager gitRepositoryManager = StoredValues.REPO_MANAGER.get(engine);
+
+ Optional<Project.NameKey> maybeParentProjectNameKey =
+ Optional.ofNullable(projectState.getProject().getParent());
+
String branch = StoredValues.getChange(engine).getDest().branch();
return new PathOwners(
accounts,
+ gitRepositoryManager,
repository,
+ maybeParentProjectNameKey,
settings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
patchList,
settings.expandGroups());
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
index 707d1b1..b05e2b2 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -19,6 +19,7 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -33,6 +34,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.googlesource.gerrit.owners.common.Accounts;
@@ -48,13 +50,17 @@
import java.util.Set;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Repository;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@Singleton
public class GetFilesOwners implements RestReadView<RevisionResource> {
+ private static final Logger log = LoggerFactory.getLogger(GetFilesOwners.class);
private final PatchListCache patchListCache;
private final Accounts accounts;
private final AccountCache accountCache;
+ private final ProjectCache projectCache;
private final GitRepositoryManager repositoryManager;
private final PluginSettings pluginSettings;
private final GerritApi gerritApi;
@@ -64,12 +70,14 @@
PatchListCache patchListCache,
Accounts accounts,
AccountCache accountCache,
+ ProjectCache projectCache,
GitRepositoryManager repositoryManager,
PluginSettings pluginSettings,
GerritApi gerritApi) {
this.patchListCache = patchListCache;
this.accounts = accounts;
this.accountCache = accountCache;
+ this.projectCache = projectCache;
this.repositoryManager = repositoryManager;
this.pluginSettings = pluginSettings;
this.gerritApi = gerritApi;
@@ -82,6 +90,9 @@
Change change = revision.getChange();
int id = revision.getChangeResource().getChange().getChangeId();
+ Optional<Project.NameKey> maybeParentProjectNameKey =
+ projectCache.get(change.getProject()).map(p -> p.getProject().getParent());
+
try (Repository repository = repositoryManager.openRepository(change.getProject())) {
PatchList patchList = patchListCache.get(change, ps);
@@ -89,7 +100,9 @@
PathOwners owners =
new PathOwners(
accounts,
+ repositoryManager,
repository,
+ maybeParentProjectNameKey,
pluginSettings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
patchList,
pluginSettings.expandGroups());
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md
index 8dffdfb..9b74897 100644
--- a/owners/src/main/resources/Documentation/config.md
+++ b/owners/src/main/resources/Documentation/config.md
@@ -111,6 +111,8 @@
That means that in the absence of any OWNERS file in the target branch, the refs/meta/config
OWNERS is used as global default.
+If the global project OWNERS has the 'inherited: true', it will check for a global project OWNERS
+in the parent project.
## Example 1 - OWNERS file without matchers and default Gerrit submit rules
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
index cf8d4d6..0c5967b 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
@@ -18,16 +18,21 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.Response;
import com.googlesource.gerrit.owners.entities.FilesOwnersResponse;
import com.googlesource.gerrit.owners.entities.GroupOwner;
import com.googlesource.gerrit.owners.entities.Owner;
import javax.servlet.http.HttpServletResponse;
import org.apache.commons.compress.utils.Sets;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
import org.junit.Test;
@TestPlugin(name = "owners", httpModule = "com.googlesource.gerrit.owners.OwnersRestApiModule")
@@ -35,41 +40,32 @@
public class GetFilesOwnersIT extends LightweightPluginDaemonTest {
private GetFilesOwners ownersApi;
+ private Owner rootOwner;
+ private Owner projectOwner;
@Override
public void setUpTestPlugin() throws Exception {
super.setUpTestPlugin();
+ rootOwner = new Owner(admin.fullName(), admin.id().get());
+ projectOwner = new Owner(user.fullName(), user.id().get());
ownersApi = plugin.getSysInjector().getInstance(GetFilesOwners.class);
-
- // Add OWNERS file to root:
- //
- // inherited: true
- // owners:
- // - admin
- merge(
- createChange(
- testRepo,
- "master",
- "Add OWNER file",
- "OWNERS",
- "owners:\n" + "- " + admin.username() + "\n",
- ""));
}
@Test
public void shouldReturnExactFileOwners() throws Exception {
+ addOwnerFileToRoot(true);
String changeId = createChange().getChangeId();
Response<FilesOwnersResponse> resp =
assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
- assertThat(resp.value().files)
- .containsExactly("a.txt", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get())));
+ assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(rootOwner));
}
@Test
public void shouldReturnOwnersLabels() throws Exception {
+ addOwnerFileToRoot(true);
String changeId = createChange().getChangeId();
approve(changeId);
@@ -83,6 +79,7 @@
@Test
@GlobalPluginConfig(pluginName = "owners", name = "owners.expandGroups", value = "false")
public void shouldReturnResponseWithUnexpandedFileOwners() throws Exception {
+ addOwnerFileToRoot(true);
String changeId = createChange().getChangeId();
Response<FilesOwnersResponse> resp =
@@ -92,8 +89,89 @@
.containsExactly("a.txt", Sets.newHashSet(new GroupOwner(admin.username())));
}
+ @Test
+ @UseLocalDisk
+ public void shouldReturnInheritedOwnersFromProjectsOwners() throws Exception {
+ assertInheritFromProject(project);
+ }
+
+ @Test
+ @UseLocalDisk
+ public void shouldReturnInheritedOwnersFromParentProjectsOwners() throws Exception {
+ assertInheritFromProject(allProjects);
+ }
+
+ @Test
+ @UseLocalDisk
+ public void shouldNotReturnInheritedOwnersFromProjectsOwners() throws Exception {
+ assertNotInheritFromProject(project);
+ }
+
+ @Test
+ @UseLocalDisk
+ public void shouldNotReturnInheritedOwnersFromParentProjectsOwners() throws Exception {
+ addOwnerFileToProjectConfig(project, false);
+ assertNotInheritFromProject(allProjects);
+ }
+
private static <T> Response<T> assertResponseOk(Response<T> response) {
assertThat(response.statusCode()).isEqualTo(HttpServletResponse.SC_OK);
return response;
}
+
+ private void assertNotInheritFromProject(Project.NameKey projectNameKey) throws Exception {
+ addOwnerFileToRoot(false);
+ addOwnerFileToProjectConfig(projectNameKey, true);
+
+ String changeId = createChange().getChangeId();
+ Response<FilesOwnersResponse> resp =
+ assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+
+ assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(rootOwner));
+ }
+
+ private void assertInheritFromProject(Project.NameKey projectNameKey) throws Exception {
+ addOwnerFileToRoot(true);
+ addOwnerFileToProjectConfig(projectNameKey, true);
+
+ String changeId = createChange().getChangeId();
+ Response<FilesOwnersResponse> resp =
+ assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+
+ assertThat(resp.value().files)
+ .containsExactly("a.txt", Sets.newHashSet(rootOwner, projectOwner));
+ }
+
+ private void addOwnerFileToProjectConfig(Project.NameKey projectNameKey, boolean inherit)
+ throws Exception {
+ TestRepository<InMemoryRepository> project = cloneProject(projectNameKey);
+ GitUtil.fetch(project, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
+ project.reset(RefNames.REFS_CONFIG);
+ pushFactory
+ .create(
+ admin.newIdent(),
+ project,
+ "Add OWNER file",
+ "OWNERS",
+ String.format(
+ "inherited: %s\nmatchers:\n" + "- suffix: .txt\n owners:\n - %s\n",
+ inherit, user.email()))
+ .to(RefNames.REFS_CONFIG);
+ }
+
+ private void addOwnerFileToRoot(boolean inherit) throws Exception {
+ // Add OWNERS file to root:
+ //
+ // inherited: true
+ // owners:
+ // - admin
+ merge(
+ createChange(
+ testRepo,
+ "master",
+ "Add OWNER file",
+ "OWNERS",
+ String.format("inherited: %s\nowners:\n- %s\n", inherit, admin.email()),
+ ""));
+ }
}
