Merge "Load actions from both actions.config and action.config"
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/workflow/RuleBase.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/workflow/RuleBase.java
index 101dfa6..685a639 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/hooks/workflow/RuleBase.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/workflow/RuleBase.java
@@ -15,16 +15,16 @@
package com.googlesource.gerrit.plugins.hooks.workflow;
import java.io.File;
+import java.io.IOException;
import java.util.Collection;
-import java.util.Collections;
import com.google.common.collect.Lists;
import com.google.gerrit.server.config.SitePath;
import com.google.inject.Inject;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.storage.file.FileBasedConfig;
import org.eclipse.jgit.util.FS;
-
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -38,7 +38,7 @@
* File (relative to site) to load rules from
*/
private static final String ITS_CONFIG_FILE = "etc" + File.separatorChar +
- "its" + File.separator + "action.config";
+ "its" + File.separator + "actions.config";
/**
* The section for rules within {@link #ITS_CONFIG_FILE}
@@ -69,22 +69,26 @@
this.ruleFactory = ruleFactory;
this.conditionFactory = conditionFactory;
this.actionRequestFactory = actionRequestFactory;
- loadRules();
+ reloadRules();
}
/**
- * Loads the rules for the RuleBase.
+ * Adds rules from a file to the the RuleBase.
+ * <p>
+ * If the given file does not exist, it is silently ignored
*
- * Consider using {@link #loadRules()@}, as that method only loads the rules,
- * if they have not yet been loaded.
+ * @param ruleFile File from which to read the rules
*/
- private void forceLoadRules() throws Exception {
- File configFile = new File(sitePath, ITS_CONFIG_FILE);
- if (configFile.exists()) {
- FileBasedConfig cfg = new FileBasedConfig(configFile, FS.DETECTED);
- cfg.load();
+ private void addRulesFromFile(File ruleFile) {
+ if (ruleFile.exists()) {
+ FileBasedConfig cfg = new FileBasedConfig(ruleFile, FS.DETECTED);
+ try {
+ cfg.load();
+ } catch (IOException | ConfigInvalidException e) {
+ log.error("Invalid ITS action configuration", e);
+ return;
+ }
- rules = Lists.newArrayList();
Collection<String> subsections = cfg.getSubsections(RULE_SECTION);
for (String subsection : subsections) {
Rule rule = ruleFactory.create(subsection);
@@ -106,24 +110,36 @@
rules.add(rule);
}
} else {
- // configFile does not exist.
- log.warn("ITS actions configuration file (" + configFile + ") does not exist.");
- rules = Collections.emptySet();
+ log.warn("ITS actions configuration file (" + ruleFile + ") does not exist.");
}
}
/**
- * Loads the rules for the RuleBase, if they have not yet been loaded.
+ * Loads the rules for the RuleBase.
*/
- private void loadRules() {
- if (rules == null) {
- try {
- forceLoadRules();
- } catch (Exception e) {
- log.error("Invalid ITS action configuration", e);
- rules = Collections.emptySet();
- }
+ private void reloadRules() {
+ rules = Lists.newArrayList();
+
+ // Add rules from file with typo in filename
+ //
+ // While the documentation called for "actions.config" (Trailing "s" in
+ // "actions"), the code previously only loaded "action.config" (No
+ // trailing "s" in "action"). To give users time to gracefully migrate to
+ // "actions.config" (with trailing "s", we (for now) load files from both
+ // locations, but consider "actions.config" (with trailing "s" the
+ // canonical place.
+ File faultyNameRuleFile = new File(sitePath, "etc" + File.separatorChar
+ + "its" + File.separator + "action.config");
+ if (faultyNameRuleFile.exists()) {
+ log.warn("Loading rules from deprecated 'etc/its/action.config' (No "
+ + "trailing 's' in 'action'). Please migrate to "
+ + "'etc/its/actions.config' (Trailing 's' in 'actions').");
+ addRulesFromFile(faultyNameRuleFile);
}
+
+ // Add global rules
+ File globalRuleFile = new File(sitePath, ITS_CONFIG_FILE);
+ addRulesFromFile(globalRuleFile);
}
/**
diff --git a/src/test/java/com/googlesource/gerrit/plugins/hooks/workflow/RuleBaseTest.java b/src/test/java/com/googlesource/gerrit/plugins/hooks/workflow/RuleBaseTest.java
index 2c6b466..b8d20d9 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/hooks/workflow/RuleBaseTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/hooks/workflow/RuleBaseTest.java
@@ -218,16 +218,103 @@
assertEquals("Matched actionRequests do not match", expected, actual);
}
+ public void testWarnExistingFaultyNameRuleBaseFile() throws IOException {
+ injectRuleBase("", true);
+
+ replayMocks();
+
+ createRuleBase();
+
+ assertLogMessageContains("Please migrate"); // Migration warning for old name
+ assertLogMessageContains("does not exist"); // For global rule file at new name
+ }
+
+ public void testSimpleFaultyNameRuleBase() throws IOException {
+ injectRuleBase("[rule \"rule1\"]\n" +
+ "\tconditionA = value1\n" +
+ "\taction = action1", true);
+
+ Rule rule1 = createMock(Rule.class);
+ expect(ruleFactory.create("rule1")).andReturn(rule1);
+
+ Condition condition1 = createMock(Condition.class);
+ expect(conditionFactory.create("conditionA", "value1")).andReturn(condition1);
+ rule1.addCondition(condition1);
+
+ ActionRequest actionRequest1 = createMock(ActionRequest.class);
+ expect(actionRequestFactory.create("action1")).andReturn(actionRequest1);
+ rule1.addActionRequest(actionRequest1);
+
+ replayMocks();
+
+ createRuleBase();
+
+ assertLogMessageContains("Please migrate"); // Migration warning for old name
+ assertLogMessageContains("does not exist"); // For global rule file at new name
+ }
+
+ public void testGlobalRuleBaseFileAndFaultyNameFileAreLoaded() throws IOException {
+ injectRuleBase("[rule \"rule1\"]\n" +
+ "\taction = action1", false);
+
+ injectRuleBase("[rule \"rule2\"]\n" +
+ "\taction = action2", true);
+
+ Collection<Property> properties = Collections.emptySet();
+
+ Rule rule1 = createMock(Rule.class);
+ expect(ruleFactory.create("rule1")).andReturn(rule1);
+
+ ActionRequest actionRequest1 = createMock(ActionRequest.class);
+ expect(actionRequestFactory.create("action1")).andReturn(actionRequest1);
+ rule1.addActionRequest(actionRequest1);
+
+ List<ActionRequest> rule1Match = Lists.newArrayListWithCapacity(1);
+ rule1Match.add(actionRequest1);
+ expect(rule1.actionRequestsFor(properties)).andReturn(rule1Match);
+
+ Rule rule2 = createMock(Rule.class);
+ expect(ruleFactory.create("rule2")).andReturn(rule2);
+
+ ActionRequest actionRequest2 = createMock(ActionRequest.class);
+ expect(actionRequestFactory.create("action2")).andReturn(actionRequest2);
+ rule2.addActionRequest(actionRequest2);
+
+ List<ActionRequest> rule2Match = Lists.newArrayListWithCapacity(1);
+ rule1Match.add(actionRequest2);
+ expect(rule2.actionRequestsFor(properties)).andReturn(rule2Match);
+
+ replayMocks();
+
+ RuleBase ruleBase = createRuleBase();
+
+ Collection<ActionRequest> actual = ruleBase.actionRequestsFor(properties);
+
+ List<ActionRequest> expected = Lists.newArrayListWithCapacity(2);
+ expected.add(actionRequest1);
+ expected.add(actionRequest2);
+
+ assertEquals("Matched actionRequests do not match", expected, actual);
+
+ assertLogMessageContains("Please migrate"); // Migration warning for old name
+ }
+
private RuleBase createRuleBase() {
return injector.getInstance(RuleBase.class);
}
private void injectRuleBase(String rules) throws IOException {
+ injectRuleBase(rules, false);
+ }
+
+ private void injectRuleBase(String rules, Boolean faultyName) throws IOException {
File ruleBaseFile = new File(sitePath, "etc" + File.separatorChar + "its" +
- File.separator + "action.config");
+ File.separator + "action" + (faultyName ? "" : "s") + ".config");
File ruleBaseParentFile = ruleBaseFile.getParentFile();
- assertTrue("Failed to create parent (" + ruleBaseParentFile + ") for " +
- "rule base", ruleBaseParentFile.mkdirs());
+ if (!ruleBaseParentFile.exists()) {
+ assertTrue("Failed to create parent (" + ruleBaseParentFile + ") for " +
+ "rule base", ruleBaseParentFile.mkdirs());
+ }
FileWriter unbufferedWriter = new FileWriter(ruleBaseFile);
BufferedWriter writer = new BufferedWriter(unbufferedWriter);
writer.write(rules);
