Allow setting label types in project.config
Project owners can add a label.Label-Name section with keys
corresponding to the existing columns in the ApprovalCategory table:
[label "My-Label"]
id = MLBL
abbreviation = L
function = MaxNoBlock
copyMinScore = true
value = -1 Negative label value
value = 0 Neutral label value
value = +1 Positive label value.
Providing a label in a child project overrides all configuration and
values for that label in parent projects or in the global DB. Labels
with no values are treated as nonexistent. Thus child projects can
disable labels defined in parent projects by adding an empty section for
that label in the child project.config.
Labels are ordered in the order they appear walking from parent to child
projects, with labels in the database ordered first. Overriding a
label's configuration does not change its ordering.
Adding a label does not imply adding any permissions, so initially new
labels will not be votable. Once a label is added, it will show up in
permission dropdowns in the project access editor, so owners can add
permissions through the web UI.
This applies equally to labels produced dynamically from Prolog rules:
to make such labels votable, a project owner needs to define label
values and permissions for those labels. Since the owner will be editing
rules.pl in refs/meta/config anyway, it should be convenient enough to
also modify project.config in the same commit.
Change-Id: I94d041236d53b1fdcd45a19806df1fb605588ff2
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
index 8cf9485..a7a66b1 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
@@ -252,7 +252,7 @@
return rule;
}
- private static int parseInt(String value) {
+ public static int parseInt(String value) {
if (value.startsWith("+")) {
value = value.substring(1);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
index 1a6a5f2..e996b61 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
@@ -16,13 +16,20 @@
import static com.google.gerrit.common.data.Permission.isPermission;
+import com.google.common.base.CharMatcher;
+import com.google.common.base.Objects;
+import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
+import com.google.common.primitives.Shorts;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action;
@@ -35,6 +42,7 @@
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.mail.Address;
+import com.google.gerrit.server.workflow.CategoryFunction;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
@@ -98,6 +106,13 @@
private static final String KEY_DEFAULT = "default";
private static final String KEY_LOCAL_DEFAULT = "local-default";
+ private static final String LABEL = "label";
+ private static final String KEY_ID = "id";
+ private static final String KEY_ABBREVIATION = "abbreviation";
+ private static final String KEY_FUNCTION = "function";
+ private static final String KEY_COPY_MIN_SCORE = "copyMinScore";
+ private static final String KEY_VALUE = "value";
+
private static final SubmitType defaultSubmitAction =
SubmitType.MERGE_IF_NECESSARY;
private static final State defaultStateValue =
@@ -110,6 +125,7 @@
private Map<String, AccessSection> accessSections;
private Map<String, ContributorAgreement> contributorAgreements;
private Map<String, NotifyConfig> notifySections;
+ private Map<String, LabelType> labelSections;
private List<ValidationError> validationErrors;
private ObjectId rulesId;
@@ -208,6 +224,10 @@
return notifySections.values();
}
+ public Collection<LabelType> getLabelSections() {
+ return labelSections.values();
+ }
+
public GroupReference resolve(AccountGroup group) {
return resolve(GroupReference.forGroup(group));
}
@@ -307,6 +327,7 @@
loadContributorAgreements(rc, groupsByName);
loadAccessSections(rc, groupsByName);
loadNotifySections(rc, groupsByName);
+ loadLabelSections(rc);
}
private void loadAccountsSection(
@@ -496,6 +517,68 @@
}
}
+ private static LabelValue parseLabelValue(String src) {
+ List<String> parts = ImmutableList.copyOf(
+ Splitter.on(CharMatcher.WHITESPACE).omitEmptyStrings().limit(2)
+ .split(src));
+ if (parts.isEmpty()) {
+ throw new IllegalArgumentException("empty value");
+ }
+ return new LabelValue(
+ Shorts.checkedCast(PermissionRule.parseInt(parts.get(0))),
+ parts.get(1));
+ }
+
+ private void loadLabelSections(Config rc) throws IOException {
+ labelSections = Maps.newLinkedHashMap();
+ for (String name : rc.getSubsections(LABEL)) {
+ List<LabelValue> values = Lists.newArrayList();
+ for (String value : rc.getStringList(LABEL, name, KEY_VALUE)) {
+ try {
+ values.add(parseLabelValue(value));
+ } catch (IllegalArgumentException notValue) {
+ error(new ValidationError(PROJECT_CONFIG, String.format(
+ "Invalid %s \"%s\" for label \"%s\": %s",
+ KEY_VALUE, value, name, notValue.getMessage())));
+ }
+ }
+
+ LabelType label;
+ String id = rc.getString(LABEL, name, KEY_ID);
+ if (id == null || id.length() > 4) {
+ error(new ValidationError(PROJECT_CONFIG, String.format(
+ "Invalid label ID \"%s\" for label \"%s\": "
+ + "Label ID may have at most 4 characters", id, name)));
+ continue;
+ }
+ try {
+ label = new LabelType(id, name, values);
+ } catch (IllegalArgumentException badName) {
+ error(new ValidationError(PROJECT_CONFIG, String.format(
+ "Invalid label \"%s\"", name)));
+ continue;
+ }
+
+ String abbr = rc.getString(LABEL, name, KEY_ABBREVIATION);
+ if (abbr != null) {
+ label.setAbbreviatedName(abbr);
+ }
+
+ String functionName = Objects.firstNonNull(
+ rc.getString(LABEL, name, KEY_FUNCTION), "MaxWithBlock");
+ if (CategoryFunction.forName(functionName) != null) {
+ label.setFunctionName(functionName);
+ } else {
+ error(new ValidationError(PROJECT_CONFIG, String.format(
+ "Invalid %s for label \"%s\"", KEY_FUNCTION, name)));
+ label.setFunctionName(null);
+ }
+ label.setCopyMinScore(
+ rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, false));
+ labelSections.put(name, label);
+ }
+ }
+
private Map<String, GroupReference> readGroupList() throws IOException {
groupsByUUID = new HashMap<AccountGroup.UUID, GroupReference>();
Map<String, GroupReference> groupsByName =
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index 9cfdddd..56d4be3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -122,10 +122,10 @@
private List<SectionMatcher> allSections;
private List<SectionMatcher> localSections;
+ private LabelTypes labelTypes;
private Map<String, RefControl> refControls;
private Boolean declaredOwner;
-
@Inject
ProjectControl(@GitUploadPackGroups Set<AccountGroup.UUID> uploadGroups,
@GitReceivePackGroups Set<AccountGroup.UUID> receiveGroups,
@@ -183,7 +183,10 @@
}
public LabelTypes getLabelTypes() {
- return state.getLabelTypes();
+ if (labelTypes == null) {
+ labelTypes = state.getLabelTypes();
+ }
+ return labelTypes;
}
private boolean isHidden() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
index 5f6d4ed..1c5e079 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
@@ -18,8 +18,10 @@
import com.google.common.base.Predicate;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
@@ -51,6 +53,7 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
+import java.util.Map;
import java.util.Set;
/** Cached information on a project. */
@@ -66,7 +69,7 @@
private final PrologEnvironment.Factory envFactory;
private final GitRepositoryManager gitMgr;
private final RulesCache rulesCache;
- private final LabelTypes labelTypes;
+ private final LabelTypes dbLabelTypes;
private final ProjectConfig config;
private final Set<AccountGroup.UUID> localOwners;
@@ -104,7 +107,7 @@
this.capabilities = isAllProjects
? new CapabilityCollection(config.getAccessSection(AccessSection.GLOBAL_CAPABILITIES))
: null;
- this.labelTypes = labelTypes;
+ this.dbLabelTypes = labelTypes;
if (isAllProjects && !Permission.canBeOnAllProjects(AccessSection.ALL, Permission.OWNER)) {
localOwners = Collections.emptySet();
@@ -342,7 +345,24 @@
}
public LabelTypes getLabelTypes() {
- return labelTypes;
+ Map<String, LabelType> types = Maps.newLinkedHashMap();
+ for (LabelType type : dbLabelTypes.getLabelTypes()) {
+ types.put(type.getName(), type);
+ }
+ List<ProjectState> projects = Lists.newArrayList(tree());
+ Collections.reverse(projects);
+ for (ProjectState s : projects) {
+ for (LabelType type : s.getConfig().getLabelSections()) {
+ types.put(type.getName(), type);
+ }
+ }
+ List<LabelType> all = Lists.newArrayListWithCapacity(types.size());
+ for (LabelType type : types.values()) {
+ if (!type.getValues().isEmpty()) {
+ all.add(type);
+ }
+ }
+ return new LabelTypes(Collections.unmodifiableList(all));
}
private boolean getInheritableBoolean(Function<Project, InheritableBoolean> func) {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java
index 0616d95..506fe90 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java
@@ -22,6 +22,7 @@
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
@@ -31,31 +32,24 @@
import java.util.Set;
public class GerritCommonTest extends PrologTestCase {
- private ProjectState project;
+ private Projects projects;
@Override
public void setUp() throws Exception {
super.setUp();
-
- LabelTypes types =
- new LabelTypes(Arrays.asList(
- category("CRVW", "Code-Review",
- value(2, "Looks good to me, approved"),
- value(1, "Looks good to me, but someone else must approve"),
- value(0, "No score"),
- value(-1, "I would prefer that you didn't submit this"),
- value(-2, "Do not submit")),
- category("VRIF", "Verified", value(1, "Verified"),
- value(0, "No score"), value(-1, "Fails"))));
- ProjectConfig config = new ProjectConfig(new Project.NameKey("myproject"));
- config.createInMemory();
- project = new ProjectState(null, null, null, null, null,
- null, types, config);
-
+ projects = new Projects(new LabelTypes(Arrays.asList(
+ category("CRVW", "Code-Review",
+ value(2, "Looks good to me, approved"),
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer that you didn't submit this"),
+ value(-2, "Do not submit")),
+ category("VRIF", "Verified", value(1, "Verified"),
+ value(0, "No score"), value(-1, "Fails")))));
load("gerrit", "gerrit_common_test.pl", new AbstractModule() {
@Override
protected void configure() {
- bind(ProjectCache.class).toInstance(new Projects(project));
+ bind(ProjectCache.class).toInstance(projects);
}
});
}
@@ -64,23 +58,32 @@
protected void setUpEnvironment(PrologEnvironment env) {
env.set(StoredValues.CHANGE, new Change(
new Change.Key("Ibeef"), new Change.Id(1), new Account.Id(2),
- new Branch.NameKey(project.getProject().getNameKey(), "master")));
+ new Branch.NameKey(projects.allProjectsName, "master")));
}
private static LabelValue value(int value, String text) {
return new LabelValue((short) value, text);
}
+ private static ProjectConfig config(String name) {
+ ProjectConfig config = new ProjectConfig(new Project.NameKey(name));
+ config.createInMemory();
+ return config;
+ }
+
private static LabelType category(String id, String name,
LabelValue... values) {
return new LabelType(id, name, Arrays.asList(values));
}
private static class Projects implements ProjectCache {
- private final ProjectState project;
+ private final AllProjectsName allProjectsName;
+ private final ProjectState allProjects;
- private Projects(ProjectState project) {
- this.project = project;
+ private Projects(LabelTypes labelTypes) {
+ allProjectsName = new AllProjectsName("All-Projects");
+ allProjects = new ProjectState(this, allProjectsName, null,
+ null, null, null, labelTypes, config(allProjectsName.get()));
}
@Override
@@ -90,8 +93,8 @@
@Override
public ProjectState get(Project.NameKey projectName) {
- assertEquals(project.getProject().getNameKey(), projectName);
- return project;
+ assertEquals(allProjectsName, projectName);
+ return allProjects;
}
@Override
