From 6159adddeb2037045c9f6338ff3be2018e47dd5b Mon Sep 17 00:00:00 2001 From: Paladox none Date: Wed, 10 Apr 2019 22:27:34 +0000 Subject: [PATCH 1/6] Highlight "starlark" as python Bug: Issue 10703 Change-Id: I3cae1006eefda742a8c26cc2085475a25d675901 --- resources/com/google/gerrit/server/mime/mime-types.properties | 1 + 1 file changed, 1 insertion(+) diff --git a/resources/com/google/gerrit/server/mime/mime-types.properties b/resources/com/google/gerrit/server/mime/mime-types.properties index e4d478e4f7..71b57bbc4a 100644 --- a/resources/com/google/gerrit/server/mime/mime-types.properties +++ b/resources/com/google/gerrit/server/mime/mime-types.properties @@ -209,6 +209,7 @@ spreadsheet = text/x-spreadsheet sql = text/x-sql ss = text/x-scheme st = text/x-stsrc +starlark = text/x-python stex = text/x-stex sv = text/x-systemverilog svh = text/x-systemverilog From d7a91bc30eb424497759ac31d031c8c52b102cb6 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 10 Apr 2019 09:30:34 +0900 Subject: [PATCH 2/6] ScheduleConfigTest: Extend tests for schedule interval Test that start time works as expected when the interval is specified with "hour", "day" and "week" in addition to "h", "d" and "w". Change-Id: If09324b1f32fd5858362afe0762c6f9b4247ff1e --- .../com/google/gerrit/server/config/ScheduleConfigTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/javatests/com/google/gerrit/server/config/ScheduleConfigTest.java b/javatests/com/google/gerrit/server/config/ScheduleConfigTest.java index 70893a9401..7e0e752193 100644 --- a/javatests/com/google/gerrit/server/config/ScheduleConfigTest.java +++ b/javatests/com/google/gerrit/server/config/ScheduleConfigTest.java @@ -40,15 +40,18 @@ public class ScheduleConfigTest { @Test public void initialDelay() throws Exception { assertThat(initialDelay("11:00", "1h")).isEqualTo(ms(1, HOURS)); + assertThat(initialDelay("11:00", "1 hour")).isEqualTo(ms(1, HOURS)); assertThat(initialDelay("05:30", "1h")).isEqualTo(ms(30, MINUTES)); assertThat(initialDelay("09:30", "1h")).isEqualTo(ms(30, MINUTES)); assertThat(initialDelay("13:30", "1h")).isEqualTo(ms(30, MINUTES)); assertThat(initialDelay("13:59", "1h")).isEqualTo(ms(59, MINUTES)); assertThat(initialDelay("11:00", "1d")).isEqualTo(ms(1, HOURS)); + assertThat(initialDelay("11:00", "1 day")).isEqualTo(ms(1, HOURS)); assertThat(initialDelay("05:30", "1d")).isEqualTo(ms(19, HOURS) + ms(30, MINUTES)); assertThat(initialDelay("11:00", "1w")).isEqualTo(ms(1, HOURS)); + assertThat(initialDelay("11:00", "1 week")).isEqualTo(ms(1, HOURS)); assertThat(initialDelay("05:30", "1w")).isEqualTo(ms(7, DAYS) - ms(4, HOURS) - ms(30, MINUTES)); assertThat(initialDelay("Mon 11:00", "1w")).isEqualTo(ms(3, DAYS) + ms(1, HOURS)); From 18f388acccff1c9383ae8fb07e98426c725af4ed Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 10 Apr 2019 09:36:18 +0900 Subject: [PATCH 3/6] Clarify that schedule config's startTime hour must be zero-padded Configuring a startTime as "6:00" is incorrect; it should be "06:00". Clarify this in the documentation and fix the Javadoc that shows an incorrect value. Also add a test. Change-Id: I8e72d42088163b6659204913e8b0685bd957c3d8 --- Documentation/config-gerrit.txt | 2 +- java/com/google/gerrit/server/config/ScheduleConfig.java | 2 +- .../com/google/gerrit/server/config/ScheduleConfigTest.java | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 76c73bf4f0..160d1d7a43 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -5064,7 +5064,7 @@ The placeholders can have the following values: + The time zone cannot be specified but is always the system default -time zone. +time zone. Hours must be zero-padded, i.e. `06:00` rather than `6:00`. The section (and optionally the subsection) in which the `interval` and `startTime` keys must be set depends on the background job for which a diff --git a/java/com/google/gerrit/server/config/ScheduleConfig.java b/java/com/google/gerrit/server/config/ScheduleConfig.java index c5f53b3659..43ffa85d9d 100644 --- a/java/com/google/gerrit/server/config/ScheduleConfig.java +++ b/java/com/google/gerrit/server/config/ScheduleConfig.java @@ -91,7 +91,7 @@ import org.eclipse.jgit.lib.Config; * executions are {@code Wed 10:30}, {@code Fri 10:30}. etc. *
  • * 
    - * foo.startTime = 6:00
+ * foo.startTime = 06:00
  * foo.interval = 1 day
  *
    * Assuming that the server is started on {@code Mon 7:00} then this yields the first run on diff --git a/javatests/com/google/gerrit/server/config/ScheduleConfigTest.java b/javatests/com/google/gerrit/server/config/ScheduleConfigTest.java index 7e0e752193..55f0374b56 100644 --- a/javatests/com/google/gerrit/server/config/ScheduleConfigTest.java +++ b/javatests/com/google/gerrit/server/config/ScheduleConfigTest.java @@ -202,6 +202,9 @@ public class ScheduleConfigTest { rc.setString("a", null, ScheduleConfig.KEY_STARTTIME, "0100"); assertThat(ScheduleConfig.builder(rc, "a").buildSchedule()).isEmpty(); + + rc.setString("a", null, ScheduleConfig.KEY_STARTTIME, "1:00"); + assertThat(ScheduleConfig.builder(rc, "a").buildSchedule()).isEmpty(); } @Test From bd37c5c86756a2cb4efdb0b33f67b17e83b16147 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 10 Apr 2019 10:02:03 +0900 Subject: [PATCH 4/6] ScheduleConfig: Log at severe when startTime or interval cannot be parsed If an invalid startTime or interval is configured, the schedule config is ignored and a message is logged: Invalid schedule configuration for "name" is ignored However, this doesn't give any indication of what was wrong with the configuration. Also log the exception that was raised when attempting to parse the value. Change-Id: I8855b39700c652b378ef0686fa0d885b7d6050b8 --- java/com/google/gerrit/server/config/ScheduleConfig.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/java/com/google/gerrit/server/config/ScheduleConfig.java b/java/com/google/gerrit/server/config/ScheduleConfig.java index 43ffa85d9d..e2c7e9cb9f 100644 --- a/java/com/google/gerrit/server/config/ScheduleConfig.java +++ b/java/com/google/gerrit/server/config/ScheduleConfig.java @@ -216,6 +216,9 @@ public abstract class ScheduleConfig { return ConfigUtil.getTimeUnit( rc, section, subsection, keyInterval, MISSING_CONFIG, TimeUnit.MILLISECONDS); } catch (IllegalArgumentException e) { + // We only need to log the exception message; it already includes the + // section.subsection.key and bad value. + logger.atSevere().log("%s", e.getMessage()); return INVALID_CONFIG; } } @@ -258,6 +261,7 @@ public abstract class ScheduleConfig { } return delay; } catch (DateTimeParseException e) { + logger.atSevere().log("Invalid start time: %s", e.getMessage()); return INVALID_CONFIG; } } From af904cad5aa92088cc45a34ddd2be48284513d70 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 10 Apr 2019 10:23:00 +0900 Subject: [PATCH 5/6] ScheduleConfig: Explicitly log when interval or initial delay value is invalid If the interval is configured as "0" it doesn't fail to parse but it still considered invalid. In this case the log message states that the invalid configuration is ignored, but not why it is invalid. Explicitly log this case. Change-Id: I69b8f2ee821c7571cf065a08eeff873e022eb4b0 --- .../google/gerrit/server/config/ScheduleConfig.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/config/ScheduleConfig.java b/java/com/google/gerrit/server/config/ScheduleConfig.java index e2c7e9cb9f..963107a239 100644 --- a/java/com/google/gerrit/server/config/ScheduleConfig.java +++ b/java/com/google/gerrit/server/config/ScheduleConfig.java @@ -174,7 +174,18 @@ public abstract class ScheduleConfig { return true; } - if (interval <= 0 || initialDelay < 0) { + if (interval != INVALID_CONFIG && interval <= 0) { + logger.atSevere().log("Invalid interval value \"%d\" for \"%s\": must be > 0", interval, key); + interval = INVALID_CONFIG; + } + + if (initialDelay != INVALID_CONFIG && initialDelay < 0) { + logger.atSevere().log( + "Invalid initial delay value \"%d\" for \"%s\": must be >= 0", initialDelay, key); + initialDelay = INVALID_CONFIG; + } + + if (interval == INVALID_CONFIG || initialDelay == INVALID_CONFIG) { logger.atSevere().log("Invalid schedule configuration for \"%s\" is ignored. ", key); return true; } From b09858031f9b304f25597612f5532f06283a36fa Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 11 Apr 2019 14:18:14 +0900 Subject: [PATCH 6/6] SiteProgram: Normalize site path Normalize the site path, i.e. remove redundant elements, from the path passed via the constructor or command line. Change-Id: Ied03c8916a5598945fea5740f259fafcd11fa36e --- java/com/google/gerrit/pgm/util/SiteProgram.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/java/com/google/gerrit/pgm/util/SiteProgram.java b/java/com/google/gerrit/pgm/util/SiteProgram.java index 1338efbe02..5930c6c831 100644 --- a/java/com/google/gerrit/pgm/util/SiteProgram.java +++ b/java/com/google/gerrit/pgm/util/SiteProgram.java @@ -71,7 +71,7 @@ public abstract class SiteProgram extends AbstractProgram { aliases = {"-d"}, usage = "Local directory containing site data") private void setSitePath(String path) { - sitePath = Paths.get(path); + sitePath = Paths.get(path).normalize(); } protected Provider dsProvider; @@ -80,13 +80,13 @@ public abstract class SiteProgram extends AbstractProgram { protected SiteProgram() {} - protected SiteProgram(Path sitePath) { - this.sitePath = sitePath; + protected SiteProgram(Path sitePath, Provider dsProvider) { + this.sitePath = sitePath.normalize(); + this.dsProvider = dsProvider; } - protected SiteProgram(Path sitePath, Provider dsProvider) { - this.sitePath = sitePath; - this.dsProvider = dsProvider; + protected SiteProgram(Path sitePath) { + this(sitePath, null); } /** @return the site path specified on the command line. */