Merge "Clean up Validator code"
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
index 537df74..20b5d11 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
@@ -101,12 +101,8 @@
.getFromProjectConfigWithInheritance(projectState, PLUGIN_NAME)
.getString(OWNERS_FILE_NAME, OWNERS);
if (name.trim().equals("")) {
- logger.atSevere().log(
- "Project %s has wrong %s: \"%s\" for %s"
- + projectState.getProject()
- + OWNERS_FILE_NAME
- + name
- + getChangeId(c));
+ logger.atSevere().log("Project %s has wrong %s: \"%s\" for %s",
+ projectState.getProject(), OWNERS_FILE_NAME, name, getChangeId(c));
return OWNERS;
}
return name;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
index 59887c9..23cc28a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -107,7 +107,8 @@
String found = "Found";
if (content.isEmpty()) {
String changeId = Config.getChangeId(changeData);
- logger.atSevere().log("Missing root %s for %s", ownersFileName, changeId);
+ logger.atSevere().log("Missing root %s for %s of %s",
+ ownersFileName, changeId, projectName);
found = "Missing";
}
logs.add(found + " root " + ownersFileName);
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index 25103e4..838234b 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -1,6 +1,133 @@
Configuration
=============
+Here we show first an example use case followed by more
+details of each configuration rule and variable.
+
+## Example from AOSP
+
+The **find-owners** plugin is used in Android Open Source Project (AOSP).
+Here is [one AOSP change](https://android-review.googlesource.com/c/platform/build/+/734949)
+that modified multiple OWNERS files.
+If you sign in to the AOSP Gerrit server,
+you can see and click the `[FIND OWNERS]` button and
+it will pop up a window with *owners* of the changed files.
+
+The AOSP Gerrit server and projects are configured with the
+following steps:
+
+1. Enable the plugin in Gerrit configuration file **gerrit.config**:
+
+ ```text
+ [plugin "find-owners"]
+ enable = true
+ maxCacheAge = 30
+ maxCacheSize = 2000
+ alwaysShowButton = true
+ ```
+
+ * **maxCacheAge** is the number of seconds owners info will stay in
+ a cache before refreshed. This reduces repeated access to the same
+ OWNERS files in a repository. If it is set to 0 (default), there will
+ be no cache.
+ * **maxCacheSize** limits the number of owners info stored in the
+ cache to reduce memory footprint.
+ * **alwaysShowButton** parameter is useful for older Gerrit UI.
+ Current Gerrit UI always displays the `[FIND OWNERS]` button.
+
+2. Enable the upload validator in **project.config** of
+ the **All-Projects** project, in the **refs/meta/config** branch:
+
+ ```text
+ [plugin "find-owners"]
+ rejectErrorInOwners = true
+ ```
+
+ * The upload validator checks basic syntax of uploaded OWNERS files.
+ * It also checks if all email addresses used in OWNERS files
+ belong to active Gerrit accounts.
+ * All other Gerrit projects inherit from **All-Projects**,
+ so they have the same enabled upload validator.
+
+2. Optionally redefine **OWNERS** file name in **project.config** of
+ some projects, in the **refs/meta/config** branch:
+
+ ```text
+ [plugin "find-owners"]
+ ownersFileName = OWNERS.android
+ ```
+
+ * The AOSP **platform/external/v8** project keeps a copy of upstream
+ source from https://github.com/v8/v8.
+ * The v8 upstream source already has its **OWNERS** files
+ that do not work with AOSP Gerrit, because the Email addresses
+ in those files are not all active developers for AOSP.
+ So we need to use different *owners* files,
+ with the **OWNERS.android** file name.
+
+4. Call the submit filters in **rules.pl** of **All-Projects**,
+ in the **refs/meta/config** branch:
+
+ ```prolog
+ % Special projects, branches, user accounts can opt out owners review.
+ % To disable all find_owners rules, add opt_out_find_owners :- true.
+ opt_out_find_owners :-
+ gerrit:change_branch('refs/heads/pie-gsi').
+
+ % Special projects, branches, user accounts can opt in owners review.
+ % To default to find_owners rules, add opt_in_find_owners :- true.
+ opt_in_find_owners :- true.
+
+ % If opt_out_find_owners is true, remove all 'Owner-Review-Vote' label;
+ % else if opt_in_find_owners is true, call find_owners:submit_filter;
+ % else default to no find_owners filter.
+ check_find_owners(In, Out) :-
+ ( opt_out_find_owners -> find_owners:remove_need_label(In, Temp)
+ ; opt_in_find_owners -> find_owners:submit_filter(In, Temp)
+ ; In = Temp
+ ),
+ Temp =.. [submit | A],
+ change_find_owners_labels(A, B),
+ Out =.. [submit | B].
+
+ submit_filter(In, Out) :-
+ In =.. [submit | A],
+ check_drno_review(A, B),
+ check_api_review(B, C),
+ check_qualcomm_review(C, D),
+ Temp =.. [submit | D],
+ check_find_owners(Temp, Out).
+
+ % Remove useless label('Owner-Approved',_) after final filter.
+ % Change optional label('Owner-Review-Vote', may(_)) to
+ % label('Owner-Review-Vote', need(_)) to hide the Submit button.
+ change_find_owners_labels([], []).
+
+ change_find_owners_labels([H | T], R) :-
+ H = label('Owner-Approved', _), !,
+ change_find_owners_labels(T, R).
+
+ change_find_owners_labels([H1 | T], [H2 | R]) :-
+ H1 = label('Owner-Review-Vote', may(_)), !,
+ H2 = label('Owner-Review-Vote', need(_)),
+ change_find_owners_labels(T, R).
+
+ change_find_owners_labels([H | T], [H | R]) :-
+ change_find_owners_labels(T, R).
+
+ ```
+
+ * With the predefined Gerrit Prolog rules, any project, branch, or
+ user can be matched and added to the **opt_out_find_owners**
+ or **opt_in_find_owners** rules.
+ * If the **submit_filter** output contains
+ **label('Owner-Review-Vote', need(_))**,
+ the Gerrit change cannot be submitted.
+ * For a simpler configuration without opt-out projects,
+ just call **find_owners:submit_filter**
+ and **change_find_owners_labels**.
+
+
## The **`submit_rule`** and **`submit_filter`**
To enforce the *owner-approval-before-submit* rule, this plugin provides
@@ -45,7 +172,7 @@
(1) add missing *owners* to the reviewers list and/or
ask for owner's +1 Code-Review votes, or
(2) add `Exempt-From-Owner-Approval:` to the commit message.
- The **`[[FIND OWNERS]]`** button is useful in this situation to find
+ The **`[FIND OWNERS]`** button is useful in this situation to find
the missing *owners* or +1 votes of any changed files.
When `label('Owner-Approved', may(_))` is added to the submit rule output,
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
index 9f9f625..0ef3b3e 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -69,7 +69,7 @@
")]}' { minOwnerVoteLevel:1, addDebugMsg:false, change:"
+ info1._number
+ ", patchset:1, file2owners:{}, reviewers:[], owners:[], files:[] }";
- Cache cache = Cache.getInstance().init(0, 10); // reset, no Cache
+ Cache cache = getCache().init(0, 10); // reset, no Cache
assertThat(cache.size()).isEqualTo(0L);
// GetOwners GET API
assertThat(getOwnersResponse(info1)).isEqualTo(expected);
@@ -367,8 +367,8 @@
@Test
public void includeProjectOwnerTest() throws Exception {
// Test include directive with other project name.
- Project.NameKey pA = projectOperations.newProject().create();
- Project.NameKey pB = projectOperations.newProject().create();
+ Project.NameKey pA = newProject("PA");
+ Project.NameKey pB = newProject("PB");
String nameA = pA.get();
String nameB = pB.get();
switchProject(pA);
@@ -498,7 +498,7 @@
Multimap<String, Account.Id> email2ids = emails.getAccountsFor(wrongEmails);
for (String email : wrongEmails) {
assertThat(emails.getAccountFor(email)).isEmpty();
- assertThat(email2ids.get(email)).isEmpty();
+ assertThat(email2ids).doesNotContainKey(email);
}
}
@@ -511,17 +511,42 @@
assertThat(content).contains("\"id\": \"All-Users\",");
assertThat(content).contains(idProject("projectTest", "project"));
assertThat(content).doesNotContain(idProject("projectTest", "ProjectA"));
- projectOperations.newProject().name(name("ProjectA")).create();
+ newProject("ProjectA");
response = adminRestSession.get("/projects/?d");
assertThat(response.getEntityContent()).contains(idProject("projectTest", "ProjectA"));
}
@Test
+ public void projectInheritanceTest() throws Exception {
+ Project.NameKey pA = newProject("Project_A");
+ Project.NameKey pB = newProject("Project_B", pA);
+ Project.NameKey pC = newProject("Project_C", pB);
+ assertThat(projectOwnersFileName(pA)).isEqualTo("OWNERS");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS");
+ assertThat(projectOwnersFileName(pC)).isEqualTo("OWNERS");
+ switchProject(pA);
+ setProjectConfig("ownersFileName", "OWNERS_A");
+ assertThat(projectOwnersFileName(pA)).isEqualTo("OWNERS_A");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS_A");
+ assertThat(projectOwnersFileName(pC)).isEqualTo("OWNERS_A");
+ switchProject(pC);
+ setProjectConfig("ownersFileName", "OWNERS_C");
+ assertThat(projectOwnersFileName(pA)).isEqualTo("OWNERS_A");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS_A");
+ assertThat(projectOwnersFileName(pC)).isEqualTo("OWNERS_C");
+ switchProject(pB);
+ setProjectConfig("ownersFileName", "OWNERS_B");
+ assertThat(projectOwnersFileName(pA)).isEqualTo("OWNERS_A");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS_B");
+ assertThat(projectOwnersFileName(pC)).isEqualTo("OWNERS_C");
+ switchProject(pC);
+ }
+
+ @Test
public void ownersFileNameTest() throws Exception {
- Config.setVariables("find-owners", configFactory);
// Default project is something like ....FindOwnersIT..._project
- Project.NameKey pA = projectOperations.newProject().name(name("Project_A")).create();
- Project.NameKey pB = projectOperations.newProject().name(name("Project_B")).create();
+ Project.NameKey pA = newProject("Project_A");
+ Project.NameKey pB = newProject("Project_B");
// Add OWNERS and OWNERS.alpha file to Project_A.
switchProject(pA);
createBranch("BranchX");
@@ -538,9 +563,10 @@
PushOneCommit.Result cY = createChangeInBranch("BranchY", "cY", "tY.c", "Hello Y!");
// Default owners file name is "OWNERS".
+ assertThat(Config.OWNERS).isEqualTo("OWNERS");
assertThat(Config.getDefaultOwnersFileName()).isEqualTo("OWNERS");
- assertThat(Config.getOwnersFileName(projectCache.get(pA), null)).isEqualTo("OWNERS");
- assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS");
+ assertThat(projectOwnersFileName(pA)).isEqualTo("OWNERS");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS");
String ownerX = oneOwnerList("x@x");
String ownerY = oneOwnerList("y@y");
@@ -578,8 +604,8 @@
switchProject(pB);
setProjectConfig("ownersFileName", "OWNERS.beta");
- assertThat(Config.getOwnersFileName(projectCache.get(pA), null)).isEqualTo("OWNERS.alpha");
- assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS.beta");
+ assertThat(projectOwnersFileName(pA)).isEqualTo("OWNERS.alpha");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS.beta");
String ownerA = oneOwnerList("a@a");
String ownerB = oneOwnerList("b@b");
cAResponse = getOwnersDebugResponse(cA);
@@ -617,7 +643,7 @@
// Change back to OWNERS in Project_A
switchProject(pA);
setProjectConfig("ownersFileName", "OWNERS");
- assertThat(Config.getOwnersFileName(projectCache.get(pA), null)).isEqualTo("OWNERS");
+ assertThat(projectOwnersFileName(pA)).isEqualTo("OWNERS");
cAResponse = getOwnersDebugResponse(cA);
cBResponse = getOwnersDebugResponse(cB);
assertThat(cAResponse).contains(ownerX + ", files:[ tA.c ]");
@@ -626,7 +652,7 @@
// Change back to OWNERS.alpha in Project_B, but there is no OWNERS.alpha
switchProject(pB);
setProjectConfig("ownersFileName", "OWNERS.alpha");
- assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS.alpha");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS.alpha");
cAResponse = getOwnersDebugResponse(cA);
cBResponse = getOwnersDebugResponse(cB);
cYResponse = getOwnersDebugResponse(cY);
@@ -640,11 +666,11 @@
// Do not accept empty string or all-white-spaces for ownersFileName.
setProjectConfig("ownersFileName", " ");
- assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS");
setProjectConfig("ownersFileName", " \t ");
- assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("OWNERS");
setProjectConfig("ownersFileName", "O");
- assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("O");
+ assertThat(projectOwnersFileName(pB)).isEqualTo("O");
}
@Test
@@ -663,7 +689,7 @@
@Test
public void actionApplyTest() throws Exception {
- Cache cache = Cache.getInstance().init(0, 10);
+ Cache cache = getCache().init(0, 10);
assertThat(cache.size()).isEqualTo(0);
// TODO: create ChangeInput in a new project.
ChangeInfo changeInfo = newChangeInfo("test Action.apply");
@@ -671,8 +697,8 @@
Action.Parameters param = new Action.Parameters();
Action action =
new Action(
- "find-owners",
- null,
+ Config.PLUGIN_NAME,
+ configFactory,
null,
changeDataFactory,
accountCache,
@@ -800,7 +826,7 @@
private int checkApproval(PushOneCommit.Result r) throws Exception {
Project.NameKey project = r.getChange().project();
- Cache cache = Cache.getInstance().init(0, 0);
+ Cache cache = getCache().init(0, 0);
OwnersDb db = cache.get(true, projectCache.get(project), accountCache, emails,
repoManager, r.getChange(), 1);
Checker c = new Checker(repoManager, r.getChange(), 1);
@@ -853,4 +879,20 @@
PushOneCommit push = pushFactory.create(admin.getIdent(), testRepo, subject, fileName, content);
return push.to("refs/for/" + branch);
}
+
+ private Project.NameKey newProject(String name) {
+ return newProject(name, project);
+ }
+
+ private Project.NameKey newProject(String myName, Project.NameKey parent) {
+ return projectOperations.newProject().parent(parent).name(name(myName)).create();
+ }
+
+ private String projectOwnersFileName(Project.NameKey name) {
+ return Config.getOwnersFileName(projectCache.get(name), null);
+ }
+
+ private Cache getCache() {
+ return Cache.getInstance();
+ }
}
