Clarify and fix sharedDirectory configuration
It wasn't specified how main.sharedDirectory is resolved when it was
given as a relative path. In particular it wasn't clear if the relative
path was resolved against the current working directory of the Java
process or against the $SITE_PATH.
Resolve relative sharedDirectory against $SITE_PATH and document that
and add a test case for the same.
This change required changing the return type of the
Main.sharedDirectory method to Path to make it cleaner. This introduced
some ugliness in the test cases with the asserts as assertThat couldn't
be used on Path instances due to a bug in the Google Truth [1]. When
this gets merged into stable-2.14, we could switch to using the
PathSubject [2] introduced by DavidO as a workaround for this issue.
[1] https://github.com/google/truth/issues/285
[2] https://gerrit.googlesource.com/gerrit/+/master/gerrit-test-util/src/main/java/com/google/gerrit/extensions/common/PathSubject.java
Change-Id: I7787bad4669514e2d27b1db975e62c9e43caea36
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java
index 0f5ad15..6c56a8c 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java
@@ -22,9 +22,12 @@
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.PluginConfigFactory;
+import com.google.gerrit.server.config.SitePaths;
import com.google.inject.Inject;
import com.google.inject.ProvisionException;
import com.google.inject.Singleton;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import org.eclipse.jgit.lib.Config;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -86,9 +89,10 @@
private final Websession websession;
@Inject
- Configuration(PluginConfigFactory pluginConfigFactory, @PluginName String pluginName) {
+ Configuration(
+ PluginConfigFactory pluginConfigFactory, @PluginName String pluginName, SitePaths site) {
Config cfg = pluginConfigFactory.getGlobalPluginConfig(pluginName);
- main = new Main(cfg);
+ main = new Main(site, cfg);
peerInfo = new PeerInfo(cfg);
http = new Http(cfg);
cache = new Cache(cfg);
@@ -146,17 +150,22 @@
}
public static class Main {
- private final String sharedDirectory;
+ private final Path sharedDirectory;
- private Main(Config cfg) {
- sharedDirectory =
- Strings.emptyToNull(cfg.getString(MAIN_SECTION, null, SHARED_DIRECTORY_KEY));
- if (sharedDirectory == null) {
+ private Main(SitePaths site, Config cfg) {
+ String shared = Strings.emptyToNull(cfg.getString(MAIN_SECTION, null, SHARED_DIRECTORY_KEY));
+ if (shared == null) {
throw new ProvisionException(SHARED_DIRECTORY_KEY + " must be configured");
}
+ Path p = Paths.get(shared);
+ if (p.isAbsolute()) {
+ sharedDirectory = p;
+ } else {
+ sharedDirectory = site.resolve(shared);
+ }
}
- public String sharedDirectory() {
+ public Path sharedDirectory() {
return sharedDirectory;
}
}
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Module.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Module.java
index 11b0605..dc9ff62 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Module.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Module.java
@@ -27,7 +27,6 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.nio.file.Paths;
class Module extends AbstractModule {
private final Configuration config;
@@ -58,7 +57,7 @@
@Singleton
@SharedDirectory
Path getSharedDirectory() throws IOException {
- Path sharedDirectoryPath = Paths.get(config.main().sharedDirectory());
+ Path sharedDirectoryPath = config.main().sharedDirectory();
Files.createDirectories(sharedDirectoryPath);
return sharedDirectoryPath;
}
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index 5faa11d..ca10488 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -17,6 +17,10 @@
main.sharedDirectory
: Path to a directory accessible from both master instances.
+ When given as a relative path, then it is resolved against the $SITE_PATH
+ or Gerrit server. For example, if $SITE_PATH is "/gerrit/root" and
+ sharedDirectory is given as "shared/dir" then the real path of the shared
+ directory is "/gerrit/root/shared/dir".
peerInfo.url
: Specify the URL for the secondary (target) instance.
@@ -85,4 +89,4 @@
* mon, month, months (`1 month` is treated as `30 days`)
* y, year, years (`1 year` is treated as `365 days`)
If a time unit suffix is not specified, `hours` is assumed.
-Defaults to 24 hours.
\ No newline at end of file
+Defaults to 24 hours.
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/ConfigurationTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/ConfigurationTest.java
index bb50a4d..a0246f6 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/ConfigurationTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/ConfigurationTest.java
@@ -40,10 +40,15 @@
import static com.ericsson.gerrit.plugins.highavailability.Configuration.WEBSESSION_SECTION;
import static com.google.common.truth.Truth.assertThat;
import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.when;
import com.google.gerrit.server.config.PluginConfigFactory;
+import com.google.gerrit.server.config.SitePaths;
import com.google.inject.ProvisionException;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import org.eclipse.jgit.lib.Config;
import org.junit.Before;
import org.junit.Test;
@@ -62,22 +67,27 @@
private static final int RETRY_INTERVAL = 1000;
private static final int THREAD_POOL_SIZE = 1;
private static final String SHARED_DIRECTORY = "/some/directory";
+ private static final Path SHARED_DIR_PATH = Paths.get(SHARED_DIRECTORY);
+ private static final String RELATIVE_SHARED_DIRECTORY = "relative/dir";
+ private static final Path SITE_PATH = Paths.get("/site_path");
private static final String ERROR_MESSAGE = "some error message";
@Mock private PluginConfigFactory cfgFactoryMock;
@Mock private Config configMock;
+ private SitePaths site;
private Configuration configuration;
private String pluginName = "high-availability";
@Before
- public void setUp() {
+ public void setUp() throws IOException {
when(cfgFactoryMock.getGlobalPluginConfig(pluginName)).thenReturn(configMock);
when(configMock.getString(MAIN_SECTION, null, SHARED_DIRECTORY_KEY))
.thenReturn(SHARED_DIRECTORY);
+ site = new SitePaths(SITE_PATH);
}
private void initializeConfiguration() {
- configuration = new Configuration(cfgFactoryMock, pluginName);
+ configuration = new Configuration(cfgFactoryMock, pluginName, site);
}
@Test
@@ -270,7 +280,7 @@
@Test
public void testGetSharedDirectory() throws Exception {
initializeConfiguration();
- assertThat(configuration.main().sharedDirectory()).isEqualTo(SHARED_DIRECTORY);
+ assertEquals(configuration.main().sharedDirectory(), SHARED_DIR_PATH);
}
@Test(expected = ProvisionException.class)
@@ -280,6 +290,16 @@
}
@Test
+ public void testRelativeSharedDir() {
+ when(configMock.getString(MAIN_SECTION, null, SHARED_DIRECTORY_KEY))
+ .thenReturn(RELATIVE_SHARED_DIRECTORY);
+ initializeConfiguration();
+
+ assertEquals(
+ configuration.main().sharedDirectory(), SITE_PATH.resolve(RELATIVE_SHARED_DIRECTORY));
+ }
+
+ @Test
public void testGetCleanupInterval() throws Exception {
initializeConfiguration();
assertThat(configuration.websession().cleanupInterval()).isEqualTo(DEFAULT_CLEANUP_INTERVAL_MS);
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/ModuleTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/ModuleTest.java
index 746c694..c7760ba 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/ModuleTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/ModuleTest.java
@@ -48,7 +48,7 @@
public void shouldCreateSharedDirectoryIfItDoesNotExist() throws Exception {
File configuredDirectory = tempFolder.newFolder();
assertThat(configuredDirectory.delete()).isTrue();
- when(configMock.main().sharedDirectory()).thenReturn(configuredDirectory.getAbsolutePath());
+ when(configMock.main().sharedDirectory()).thenReturn(configuredDirectory.toPath());
Path sharedDirectory = module.getSharedDirectory();
assertThat(sharedDirectory.toFile().exists()).isTrue();
@@ -57,7 +57,7 @@
@Test(expected = IOException.class)
public void shouldThrowAnExceptionIfAnErrorOccurCreatingSharedDirectory() throws Exception {
File configuredDirectory = tempFolder.newFile();
- when(configMock.main().sharedDirectory()).thenReturn(configuredDirectory.getAbsolutePath());
+ when(configMock.main().sharedDirectory()).thenReturn(configuredDirectory.toPath());
module.getSharedDirectory();
}
