Load actions from both actions.config and action.config While documentation asked for "actions.config" (trailing "s" in "actions"), the code only loaded action.config (no trailing "s" in "action"). To make up for this mess, we load from both files, and consider "actions.config" the canonical place. Change-Id: I475f152bc14d90664ee673b5cd559d61155ab3ce
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);