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
This commit is contained in:
Dave Borowitz
2013-02-18 10:37:59 -08:00
parent c113b55efb
commit c688750827
5 changed files with 139 additions and 30 deletions

View File

@@ -252,7 +252,7 @@ public class PermissionRule implements Comparable<PermissionRule> {
return rule; return rule;
} }
private static int parseInt(String value) { public static int parseInt(String value) {
if (value.startsWith("+")) { if (value.startsWith("+")) {
value = value.substring(1); value = value.substring(1);
} }

View File

@@ -16,13 +16,20 @@ package com.google.gerrit.server.git;
import static com.google.gerrit.common.data.Permission.isPermission; 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.Lists;
import com.google.common.collect.Maps; 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.AccessSection;
import com.google.gerrit.common.data.ContributorAgreement; import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference; 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.Permission;
import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action; import com.google.gerrit.common.data.PermissionRule.Action;
@@ -35,6 +42,7 @@ import com.google.gerrit.reviewdb.client.Project.SubmitType;
import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.workflow.CategoryFunction;
import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.CommitBuilder;
@@ -98,6 +106,13 @@ public class ProjectConfig extends VersionedMetaData {
private static final String KEY_DEFAULT = "default"; private static final String KEY_DEFAULT = "default";
private static final String KEY_LOCAL_DEFAULT = "local-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 = private static final SubmitType defaultSubmitAction =
SubmitType.MERGE_IF_NECESSARY; SubmitType.MERGE_IF_NECESSARY;
private static final State defaultStateValue = private static final State defaultStateValue =
@@ -110,6 +125,7 @@ public class ProjectConfig extends VersionedMetaData {
private Map<String, AccessSection> accessSections; private Map<String, AccessSection> accessSections;
private Map<String, ContributorAgreement> contributorAgreements; private Map<String, ContributorAgreement> contributorAgreements;
private Map<String, NotifyConfig> notifySections; private Map<String, NotifyConfig> notifySections;
private Map<String, LabelType> labelSections;
private List<ValidationError> validationErrors; private List<ValidationError> validationErrors;
private ObjectId rulesId; private ObjectId rulesId;
@@ -208,6 +224,10 @@ public class ProjectConfig extends VersionedMetaData {
return notifySections.values(); return notifySections.values();
} }
public Collection<LabelType> getLabelSections() {
return labelSections.values();
}
public GroupReference resolve(AccountGroup group) { public GroupReference resolve(AccountGroup group) {
return resolve(GroupReference.forGroup(group)); return resolve(GroupReference.forGroup(group));
} }
@@ -307,6 +327,7 @@ public class ProjectConfig extends VersionedMetaData {
loadContributorAgreements(rc, groupsByName); loadContributorAgreements(rc, groupsByName);
loadAccessSections(rc, groupsByName); loadAccessSections(rc, groupsByName);
loadNotifySections(rc, groupsByName); loadNotifySections(rc, groupsByName);
loadLabelSections(rc);
} }
private void loadAccountsSection( private void loadAccountsSection(
@@ -496,6 +517,68 @@ public class ProjectConfig extends VersionedMetaData {
} }
} }
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 { private Map<String, GroupReference> readGroupList() throws IOException {
groupsByUUID = new HashMap<AccountGroup.UUID, GroupReference>(); groupsByUUID = new HashMap<AccountGroup.UUID, GroupReference>();
Map<String, GroupReference> groupsByName = Map<String, GroupReference> groupsByName =

View File

@@ -122,10 +122,10 @@ public class ProjectControl {
private List<SectionMatcher> allSections; private List<SectionMatcher> allSections;
private List<SectionMatcher> localSections; private List<SectionMatcher> localSections;
private LabelTypes labelTypes;
private Map<String, RefControl> refControls; private Map<String, RefControl> refControls;
private Boolean declaredOwner; private Boolean declaredOwner;
@Inject @Inject
ProjectControl(@GitUploadPackGroups Set<AccountGroup.UUID> uploadGroups, ProjectControl(@GitUploadPackGroups Set<AccountGroup.UUID> uploadGroups,
@GitReceivePackGroups Set<AccountGroup.UUID> receiveGroups, @GitReceivePackGroups Set<AccountGroup.UUID> receiveGroups,
@@ -183,7 +183,10 @@ public class ProjectControl {
} }
public LabelTypes getLabelTypes() { public LabelTypes getLabelTypes() {
return state.getLabelTypes(); if (labelTypes == null) {
labelTypes = state.getLabelTypes();
}
return labelTypes;
} }
private boolean isHidden() { private boolean isHidden() {

View File

@@ -18,8 +18,10 @@ import com.google.common.base.Function;
import com.google.common.base.Predicate; import com.google.common.base.Predicate;
import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
import com.google.common.collect.Lists; 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.AccessSection;
import com.google.gerrit.common.data.GroupReference; 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.LabelTypes;
import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule;
@@ -51,6 +53,7 @@ import java.util.Collections;
import java.util.HashSet; import java.util.HashSet;
import java.util.Iterator; import java.util.Iterator;
import java.util.List; import java.util.List;
import java.util.Map;
import java.util.Set; import java.util.Set;
/** Cached information on a project. */ /** Cached information on a project. */
@@ -66,7 +69,7 @@ public class ProjectState {
private final PrologEnvironment.Factory envFactory; private final PrologEnvironment.Factory envFactory;
private final GitRepositoryManager gitMgr; private final GitRepositoryManager gitMgr;
private final RulesCache rulesCache; private final RulesCache rulesCache;
private final LabelTypes labelTypes; private final LabelTypes dbLabelTypes;
private final ProjectConfig config; private final ProjectConfig config;
private final Set<AccountGroup.UUID> localOwners; private final Set<AccountGroup.UUID> localOwners;
@@ -104,7 +107,7 @@ public class ProjectState {
this.capabilities = isAllProjects this.capabilities = isAllProjects
? new CapabilityCollection(config.getAccessSection(AccessSection.GLOBAL_CAPABILITIES)) ? new CapabilityCollection(config.getAccessSection(AccessSection.GLOBAL_CAPABILITIES))
: null; : null;
this.labelTypes = labelTypes; this.dbLabelTypes = labelTypes;
if (isAllProjects && !Permission.canBeOnAllProjects(AccessSection.ALL, Permission.OWNER)) { if (isAllProjects && !Permission.canBeOnAllProjects(AccessSection.ALL, Permission.OWNER)) {
localOwners = Collections.emptySet(); localOwners = Collections.emptySet();
@@ -342,7 +345,24 @@ public class ProjectState {
} }
public LabelTypes getLabelTypes() { 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) { private boolean getInheritableBoolean(Function<Project, InheritableBoolean> func) {

View File

@@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project; 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.git.ProjectConfig;
import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.project.ProjectState;
@@ -31,14 +32,12 @@ import java.util.Arrays;
import java.util.Set; import java.util.Set;
public class GerritCommonTest extends PrologTestCase { public class GerritCommonTest extends PrologTestCase {
private ProjectState project; private Projects projects;
@Override @Override
public void setUp() throws Exception { public void setUp() throws Exception {
super.setUp(); super.setUp();
projects = new Projects(new LabelTypes(Arrays.asList(
LabelTypes types =
new LabelTypes(Arrays.asList(
category("CRVW", "Code-Review", category("CRVW", "Code-Review",
value(2, "Looks good to me, approved"), value(2, "Looks good to me, approved"),
value(1, "Looks good to me, but someone else must approve"), value(1, "Looks good to me, but someone else must approve"),
@@ -46,16 +45,11 @@ public class GerritCommonTest extends PrologTestCase {
value(-1, "I would prefer that you didn't submit this"), value(-1, "I would prefer that you didn't submit this"),
value(-2, "Do not submit")), value(-2, "Do not submit")),
category("VRIF", "Verified", value(1, "Verified"), category("VRIF", "Verified", value(1, "Verified"),
value(0, "No score"), value(-1, "Fails")))); 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);
load("gerrit", "gerrit_common_test.pl", new AbstractModule() { load("gerrit", "gerrit_common_test.pl", new AbstractModule() {
@Override @Override
protected void configure() { protected void configure() {
bind(ProjectCache.class).toInstance(new Projects(project)); bind(ProjectCache.class).toInstance(projects);
} }
}); });
} }
@@ -64,23 +58,32 @@ public class GerritCommonTest extends PrologTestCase {
protected void setUpEnvironment(PrologEnvironment env) { protected void setUpEnvironment(PrologEnvironment env) {
env.set(StoredValues.CHANGE, new Change( env.set(StoredValues.CHANGE, new Change(
new Change.Key("Ibeef"), new Change.Id(1), new Account.Id(2), 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) { private static LabelValue value(int value, String text) {
return new LabelValue((short) value, 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, private static LabelType category(String id, String name,
LabelValue... values) { LabelValue... values) {
return new LabelType(id, name, Arrays.asList(values)); return new LabelType(id, name, Arrays.asList(values));
} }
private static class Projects implements ProjectCache { private static class Projects implements ProjectCache {
private final ProjectState project; private final AllProjectsName allProjectsName;
private final ProjectState allProjects;
private Projects(ProjectState project) { private Projects(LabelTypes labelTypes) {
this.project = project; allProjectsName = new AllProjectsName("All-Projects");
allProjects = new ProjectState(this, allProjectsName, null,
null, null, null, labelTypes, config(allProjectsName.get()));
} }
@Override @Override
@@ -90,8 +93,8 @@ public class GerritCommonTest extends PrologTestCase {
@Override @Override
public ProjectState get(Project.NameKey projectName) { public ProjectState get(Project.NameKey projectName) {
assertEquals(project.getProject().getNameKey(), projectName); assertEquals(allProjectsName, projectName);
return project; return allProjects;
} }
@Override @Override