Add SubmitRule extension point
The creation of custom rules to control the submitability of changes presents major disadvantages: the rules must be written in Prolog, which is harder to master than other options, and not as modular as it could be. By providing an interface that plugins can use to implement custom presubmit rules, these disadvantages should disappear: Gerrit plugins have full access to the core systems, but also to external resources. Everything that was possible with Prolog should be possible, and easier to achieve. Plugins can implement this interface and react however they want: a plugin can for instance define a Prolog rule evaluator, while an other hardcodes the rules in the Java code. With this implementation, all the plugins implementing SubmitRule are called. If a plugin doesn't want to participate in the voting process, it just has to return an empty collection. An other way to implement a similar process would have been to use the project's config file to enable or disable each plugin, by hand. It is more explicit: we know what is enabled and where, but it is also harder to maintain: plugins wouldn't work out of the box. What format should be used to declare the config? Also, this is not retro-compatible with the Prolog rules engine, which is enabled by default _if_ a file named rules.pl exists. A similar change was proposed by Saša Živkov during the Gerrit Hackathon 2016, cf [1] and the change Ifd5d2a in particular. The main difference between our changes lies in the design. The purpose of this change (here) is to allow the use of multiple validation plugins, so that one can handle the OWNERS file while an other checks the Labels, for instance. I truely believe plugin composition is important for the pre submit rules, as it allows smaller modular plugins while allowing greater power to the end user. Unfortunately, it also makes it very hard, if not impossible, to correctly handle dynamically determined submit types. Thus, Prolog will be able to change it (for backward compatibility reasons) but new plugins won't have this option for now. (The issue here being that multiple plugins may not come to an agreement on what submit type should be used). Our SubmitRule interfaces look similar, with two slight differences: - We don't encourage throwing exceptions. Plugins should handle this case. - We don't pass a second argument "SubmitRulesFlag" It is also important to note that the second commit of this change introduces SubmitRequirements, allowing plugin authors to improve the interactions with the users, replacing the strange error messages like "Needs label: Author-Is-Maxime" with explanatory messages. The third commit introduces a change that is only required because of plugin composition: a change can only be submitted if all the plugins agree that it can be. In the third change, Saša Živkov implemented the LabelFunctions in Java. I think this is a good idea and plan to do the same. [1]: https://gerrit-review.googlesource.com/q/topic:non-prolog-submit-rules-gh16 Change-Id: Ic0731321eb9d182ddbfa27d7c08eaeea9f3155e5
This commit is contained in:
parent
5c0ec401b2
commit
a63cc6eb70
@ -2731,10 +2731,114 @@ class MyCommandInterceptor implements SshCreateCommandInterceptor {
|
||||
@Override
|
||||
public String intercept(String in) {
|
||||
return pluginName + " mycommand";
|
||||
----
|
||||
|
||||
|
||||
[[pre-submit-evaluator]]
|
||||
== Pre-submit Validation Plugins
|
||||
|
||||
Gerrit provides an extension point that enables plugins to prevent a change
|
||||
from being submitted.
|
||||
|
||||
[IMPORTANT]
|
||||
This extension point **must NOT** be used for long or slow operations, like
|
||||
calling external programs or content, running unit tests...
|
||||
Slow operations will hurt the whole Gerrit instance.
|
||||
|
||||
This can be used to implement custom rules that changes have to match to become
|
||||
submittable. A more concrete example: the Prolog rules engine can be
|
||||
implemented using this.
|
||||
|
||||
Gerrit calls the plugins once per change and caches the results. Although it is
|
||||
possible to predict when this interface will be triggered, this should not be
|
||||
considered as a feature. Plugins should only rely on the internal state of the
|
||||
ChangeData, not on external values like date and time, remote content or
|
||||
randomness.
|
||||
|
||||
Plugins are expected to support rules inheritance themselves, providing ways
|
||||
to configure it and handling the logic behind it.
|
||||
Please note that no inheritance is sometimes better than badly handled
|
||||
inheritance: mis-communication and strange behaviors caused by inheritance
|
||||
may and will confuse the users. Each plugins is responsible for handling the
|
||||
project hierarchy and taking wise actions. Gerrit does not enforce it.
|
||||
|
||||
Once Gerrit has gathered every plugins' SubmitRecords, it stores them.
|
||||
|
||||
Plugins accept or reject a given change using `SubmitRecord.Status`.
|
||||
If a change is ready to be submitted, `OK`. If it is not ready and requires
|
||||
modifications, `NOT_READY`. Other statuses are available for particular cases.
|
||||
A change can be submitted if all the plugins accept the change.
|
||||
|
||||
Plugins may also decide not to vote on a given change by returning an empty
|
||||
Collection (ie: the plugin is not enabled for this repository), or to vote
|
||||
several times (ie: one SubmitRecord per project in the hierarchy).
|
||||
The results are handled as if multiple plugins voted for the change.
|
||||
|
||||
If a plugin decides not to vote, it's name will not be displayed in the UI and
|
||||
it will not be recoded in the database.
|
||||
|
||||
.Gerrit's Pre-submit handling with three plugins
|
||||
[width="50%",cols="^m,^m,^m,^m",frame="topbot",options="header"]
|
||||
|=======================================================
|
||||
| Plugin A | Plugin B | Plugin C | Final decision
|
||||
| OK | OK | OK | OK
|
||||
| OK | OK | / | OK
|
||||
| OK | OK | RULE_ERROR | NOT_READY
|
||||
| OK | NOT_READY | OK | NOT_READY
|
||||
| NOT_READY | OK | OK | NOT_READY
|
||||
|=======================================================
|
||||
|
||||
|
||||
This makes composing plugins really easy.
|
||||
|
||||
- If a plugin places a veto on a change, it can't be submitted.
|
||||
- If a plugin isn't enabled for a project (or isn't needed for this change),
|
||||
it returns an empty collection.
|
||||
- If all the plugins answer `OK`, the change can be submitted.
|
||||
|
||||
|
||||
A more rare case, but worth documenting: if there are no installed plugins,
|
||||
the labels will be compared to the rules defined in the project's config,
|
||||
and the permission system will be used to allow or deny a submit request.
|
||||
|
||||
Some rules are defined internally to provide a common base ground (and sanity):
|
||||
changes that are marked as WIP or that are closed (abandoned, merged) can't be merged.
|
||||
|
||||
|
||||
[source, java]
|
||||
----
|
||||
import java.util.Collection;
|
||||
import com.google.gerrit.common.data.SubmitRecord;
|
||||
import com.google.gerrit.common.data.SubmitRecord.Status;
|
||||
import com.google.gerrit.server.query.change.ChangeData;
|
||||
import com.google.gerrit.server.rules.SubmitRule;
|
||||
|
||||
public class MyPluginRules implements SubmitRule {
|
||||
public Collection<SubmitRecord> evaluate(ChangeData changeData) {
|
||||
// Implement your submitability logic here
|
||||
|
||||
// Assuming we want to prevent this change from being submitted:
|
||||
SubmitRecord record;
|
||||
record.status = Status.NOT_READY;
|
||||
return record;
|
||||
}
|
||||
}
|
||||
----
|
||||
|
||||
Don't forget to register your class!
|
||||
|
||||
[source, java]
|
||||
----
|
||||
import com.google.gerrit.extensions.annotations.Exports;
|
||||
import com.google.inject.AbstractModule;
|
||||
|
||||
public class MyPluginModule extends AbstractModule {
|
||||
@Override
|
||||
protected void configure() {
|
||||
bind(SubmitRule.class).annotatedWith(Exports.named("myPlugin")).to(MyPluginRules.class);
|
||||
}
|
||||
}
|
||||
----
|
||||
|
||||
== SEE ALSO
|
||||
|
||||
|
@ -157,7 +157,6 @@ public class BatchProgramModule extends FactoryModule {
|
||||
install(new ExternalIdModule());
|
||||
install(new GroupModule());
|
||||
install(new NoteDbModule(cfg));
|
||||
install(new PrologModule());
|
||||
install(AccountCacheImpl.module());
|
||||
install(GroupCacheImpl.module());
|
||||
install(GroupIncludeCacheImpl.module());
|
||||
@ -169,7 +168,10 @@ public class BatchProgramModule extends FactoryModule {
|
||||
factory(CapabilityCollection.Factory.class);
|
||||
factory(ChangeData.AssistedFactory.class);
|
||||
factory(ProjectState.Factory.class);
|
||||
|
||||
// Submit rule evaluator
|
||||
factory(SubmitRuleEvaluator.Factory.class);
|
||||
install(new PrologModule());
|
||||
|
||||
bind(ChangeJson.Factory.class).toProvider(Providers.<ChangeJson.Factory>of(null));
|
||||
bind(EventUtil.class).toProvider(Providers.<EventUtil>of(null));
|
||||
|
@ -177,6 +177,7 @@ import com.google.gerrit.server.restapi.config.ConfigRestModule;
|
||||
import com.google.gerrit.server.restapi.group.GroupModule;
|
||||
import com.google.gerrit.server.rules.PrologModule;
|
||||
import com.google.gerrit.server.rules.RulesCache;
|
||||
import com.google.gerrit.server.rules.SubmitRule;
|
||||
import com.google.gerrit.server.ssh.SshAddressesModule;
|
||||
import com.google.gerrit.server.tools.ToolsCatalog;
|
||||
import com.google.gerrit.server.update.BatchUpdate;
|
||||
@ -391,6 +392,7 @@ public class GerritGlobalModule extends FactoryModule {
|
||||
DynamicSet.setOf(binder(), ActionVisitor.class);
|
||||
DynamicItem.itemOf(binder(), MergeSuperSetComputation.class);
|
||||
DynamicItem.itemOf(binder(), ProjectNameLockManager.class);
|
||||
DynamicSet.setOf(binder(), SubmitRule.class);
|
||||
|
||||
DynamicMap.mapOf(binder(), MailFilter.class);
|
||||
bind(MailFilter.class).annotatedWith(Exports.named("ListMailFilter")).to(ListMailFilter.class);
|
||||
|
@ -728,6 +728,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
|
||||
msg.append('\n');
|
||||
}
|
||||
}
|
||||
// TODO(maximeg) We might want to list plugins that validated this submission.
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -14,17 +14,21 @@
|
||||
|
||||
package com.google.gerrit.server.project;
|
||||
|
||||
import com.google.common.collect.ImmutableList;
|
||||
import com.google.gerrit.common.data.SubmitRecord;
|
||||
import com.google.gerrit.common.data.SubmitTypeRecord;
|
||||
import com.google.gerrit.extensions.registration.DynamicSet;
|
||||
import com.google.gerrit.reviewdb.client.Change;
|
||||
import com.google.gerrit.server.query.change.ChangeData;
|
||||
import com.google.gerrit.server.rules.PrologRule;
|
||||
import com.google.gerrit.server.rules.SubmitRule;
|
||||
import com.google.gwtorm.server.OrmException;
|
||||
import com.google.inject.Inject;
|
||||
import com.google.inject.assistedinject.Assisted;
|
||||
import java.util.Collection;
|
||||
import java.util.Collections;
|
||||
import java.util.List;
|
||||
import java.util.stream.Collectors;
|
||||
import java.util.stream.StreamSupport;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
|
||||
@ -38,6 +42,7 @@ public class SubmitRuleEvaluator {
|
||||
|
||||
private final ProjectCache projectCache;
|
||||
private final PrologRule prologRule;
|
||||
private final DynamicSet<SubmitRule> submitRules;
|
||||
private final SubmitRuleOptions opts;
|
||||
|
||||
public interface Factory {
|
||||
@ -47,9 +52,13 @@ public class SubmitRuleEvaluator {
|
||||
|
||||
@Inject
|
||||
private SubmitRuleEvaluator(
|
||||
ProjectCache projectCache, PrologRule prologRule, @Assisted SubmitRuleOptions options) {
|
||||
ProjectCache projectCache,
|
||||
PrologRule prologRule,
|
||||
DynamicSet<SubmitRule> submitRules,
|
||||
@Assisted SubmitRuleOptions options) {
|
||||
this.projectCache = projectCache;
|
||||
this.prologRule = prologRule;
|
||||
this.submitRules = submitRules;
|
||||
|
||||
this.opts = options;
|
||||
}
|
||||
@ -99,7 +108,12 @@ public class SubmitRuleEvaluator {
|
||||
return Collections.singletonList(rec);
|
||||
}
|
||||
|
||||
return ImmutableList.copyOf(prologRule.evaluate(cd, opts));
|
||||
// We evaluate all the plugin-defined evaluators,
|
||||
// and then we collect the results in one list.
|
||||
return StreamSupport.stream(submitRules.spliterator(), false)
|
||||
.map(s -> s.evaluate(cd, opts))
|
||||
.flatMap(Collection::stream)
|
||||
.collect(Collectors.toList());
|
||||
}
|
||||
|
||||
private List<SubmitRecord> ruleError(String err, Exception e) {
|
||||
|
@ -14,6 +14,7 @@
|
||||
|
||||
package com.google.gerrit.server.rules;
|
||||
|
||||
import com.google.gerrit.extensions.annotations.Exports;
|
||||
import com.google.gerrit.extensions.config.FactoryModule;
|
||||
import com.google.gerrit.extensions.registration.DynamicSet;
|
||||
|
||||
@ -22,8 +23,9 @@ public class PrologModule extends FactoryModule {
|
||||
protected void configure() {
|
||||
install(new EnvironmentModule());
|
||||
bind(PrologEnvironment.Args.class);
|
||||
bind(PrologRule.class);
|
||||
factory(PrologRuleEvaluator.Factory.class);
|
||||
|
||||
bind(SubmitRule.class).annotatedWith(Exports.named("PrologRule")).to(PrologRule.class);
|
||||
}
|
||||
|
||||
static class EnvironmentModule extends FactoryModule {
|
||||
|
@ -24,7 +24,7 @@ import com.google.inject.Singleton;
|
||||
import java.util.Collection;
|
||||
|
||||
@Singleton
|
||||
public class PrologRule {
|
||||
public class PrologRule implements SubmitRule {
|
||||
|
||||
private final Factory factory;
|
||||
|
||||
@ -33,6 +33,7 @@ public class PrologRule {
|
||||
this.factory = factory;
|
||||
}
|
||||
|
||||
@Override
|
||||
public Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions opts) {
|
||||
return getEvaluator(cd, opts).evaluate();
|
||||
}
|
||||
|
@ -261,6 +261,10 @@ public class PrologRuleEvaluator {
|
||||
}
|
||||
Collections.reverse(out);
|
||||
|
||||
// This transformation is required to adapt Prolog's behavior to the way Gerrit handles
|
||||
// SubmitRecords, as defined in the SubmitRecord#allRecordsOK method.
|
||||
// When several rules are defined in Prolog, they are all matched to a SubmitRecord. We want
|
||||
// the change to be submittable when at least one result is OK.
|
||||
if (foundOk) {
|
||||
for (SubmitRecord record : out) {
|
||||
record.status = SubmitRecord.Status.OK;
|
||||
|
44
java/com/google/gerrit/server/rules/SubmitRule.java
Normal file
44
java/com/google/gerrit/server/rules/SubmitRule.java
Normal file
@ -0,0 +1,44 @@
|
||||
// Copyright (C) 2018 The Android Open Source Project
|
||||
//
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
package com.google.gerrit.server.rules;
|
||||
|
||||
import com.google.gerrit.common.data.SubmitRecord;
|
||||
import com.google.gerrit.extensions.annotations.ExtensionPoint;
|
||||
import com.google.gerrit.server.project.SubmitRuleOptions;
|
||||
import com.google.gerrit.server.query.change.ChangeData;
|
||||
import java.util.Collection;
|
||||
|
||||
/**
|
||||
* Allows plugins to decide whether a change is ready to be submitted or not.
|
||||
*
|
||||
* <p>For a given {@link ChangeData}, each plugin is called and returns a {@link Collection} of
|
||||
* {@link SubmitRecord}. This collection can be empty, or contain one or several values.
|
||||
*
|
||||
* <p>A Change can only be submitted if all the plugins give their consent.
|
||||
*
|
||||
* <p>Each {@link SubmitRecord} represents a decision made by the plugin. If the plugin rejects a
|
||||
* change, it should hold valuable informations to help the end user understand and correct the
|
||||
* blocking points.
|
||||
*
|
||||
* <p>It should be noted that each plugin can handle rules inheritance.
|
||||
*
|
||||
* <p>This interface should be used to write pre-submit validation rules. This includes both simple
|
||||
* checks, coded in Java, and more complex fully fledged expression evaluators (think: Prolog,
|
||||
* JavaCC, or even JavaScript rules).
|
||||
*/
|
||||
@ExtensionPoint
|
||||
public interface SubmitRule {
|
||||
/** Returns a {@link Collection} of {@link SubmitRecord} status for the change. */
|
||||
Collection<SubmitRecord> evaluate(ChangeData changeData, SubmitRuleOptions options);
|
||||
}
|
Loading…
x
Reference in New Issue
Block a user