Fix disabling of healthchecks When a specific healthcheck is disabled, the HTTP status code should be 200. Make sure that disabled tests are checked during integration tests. Bug: Issue 10574 Change-Id: Ia4e46e49873279f63fcbab925f49b68b70d230a6
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java index b92588b..4e4fab1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java
@@ -60,7 +60,7 @@ } @VisibleForTesting - public HealthCheckConfig(String configText) { + HealthCheckConfig(String configText) { config = new Config(); if (!Strings.isNullOrEmpty(configText)) { try { @@ -73,6 +73,11 @@ allUsersName = new AllUsersName("All-Users"); } + @VisibleForTesting + void fromText(String configText) throws ConfigInvalidException { + config.fromText(configText); + } + public long getTimeout() { return getTimeout(null); }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java index 26a2042..c508222 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
@@ -51,7 +51,7 @@ .filter( res -> res instanceof HealthCheck.StatusSummary - && ((HealthCheck.StatusSummary) res).result != HealthCheck.Result.PASSED) + && ((HealthCheck.StatusSummary) res).isFailure()) .findFirst() .isPresent()) { return HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java index e8aeb95..a6eae79 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
@@ -29,14 +29,14 @@ private final String name; private final ListeningExecutorService executor; protected StatusSummary latestStatus; - protected boolean enabled; + protected HealthCheckConfig config; protected AbstractHealthCheck( ListeningExecutorService executor, HealthCheckConfig config, String name) { this.executor = executor; this.name = name; this.timeout = config.getTimeout(name); - this.enabled = config.healthCheckEnabled(name); + this.config = config; this.latestStatus = StatusSummary.INITIAL_STATUS; } @@ -47,6 +47,7 @@ @Override public StatusSummary run() { + boolean enabled = config.healthCheckEnabled(name); final long ts = System.currentTimeMillis(); ListenableFuture<StatusSummary> resultFuture = executor.submit(
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java index 858e142..de571d1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java
@@ -39,7 +39,7 @@ public final long ts; public final long elapsed; public static final Set<Result> failingResults = - new HashSet(Arrays.asList(Result.FAILED, Result.TIMEOUT)); + new HashSet<>(Arrays.asList(Result.FAILED, Result.TIMEOUT)); public StatusSummary(Result result, long ts, long elapsed) { this.result = result;
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java index 9f481ff..b244b65 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java
@@ -16,6 +16,7 @@ import static com.google.common.net.HttpHeaders.CONTENT_TYPE; import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.AUTH; import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.JGIT; import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.QUERYCHANGES; import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.REVIEWDB; @@ -24,12 +25,11 @@ import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestPlugin; -import com.google.gerrit.extensions.annotations.PluginName; import com.google.gson.Gson; import com.google.gson.JsonObject; -import com.google.inject.AbstractModule; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames; import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.junit.Before; import org.junit.Test; @@ -44,17 +44,7 @@ public void setUp() throws Exception { super.setUp(); - config = - server - .getTestInjector() - .createChildInjector( - new AbstractModule() { - @Override - protected void configure() { - bind(String.class).annotatedWith(PluginName.class).toInstance("healthcheck"); - } - }) - .getInstance(HealthCheckConfig.class); + config = plugin.getSysInjector().getInstance(HealthCheckConfig.class); int numChanges = config.getLimit(HealthCheckNames.QUERYCHANGES); for (int i = 0; i < numChanges; i++) { createChange("refs/for/master"); @@ -75,21 +65,52 @@ @Test public void shouldReturnReviewDbCheck() throws Exception { RestResponse resp = getHealthCheckStatus(); + resp.assertOK(); + assertCheckResult(getResponseJson(resp), REVIEWDB, "passed"); + } - JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); + @Test + public void shouldReturnReviewDbCheckAsDisabled() throws Exception { + disableCheck(REVIEWDB); + RestResponse resp = getHealthCheckStatus(); - assertCheckResult(respPayload, REVIEWDB, "passed"); + resp.assertOK(); + assertCheckResult(getResponseJson(resp), REVIEWDB, "disabled"); } @Test public void shouldReturnJGitCheck() throws Exception { RestResponse resp = getHealthCheckStatus(); + resp.assertOK(); + assertCheckResult(getResponseJson(resp), JGIT, "passed"); + } - JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); + @Test + public void shouldReturnJGitCheckAsDisabled() throws Exception { + disableCheck(JGIT); + RestResponse resp = getHealthCheckStatus(); - assertCheckResult(respPayload, JGIT, "passed"); + resp.assertOK(); + assertCheckResult(getResponseJson(resp), JGIT, "disabled"); + } + + @Test + public void shouldReturnAuthCheck() throws Exception { + RestResponse resp = getHealthCheckStatus(); + + resp.assertOK(); + assertCheckResult(getResponseJson(resp), AUTH, "passed"); + } + + @Test + public void shouldReturnAuthCheckAsDisabled() throws Exception { + disableCheck(AUTH); + RestResponse resp = getHealthCheckStatus(); + + resp.assertOK(); + assertCheckResult(getResponseJson(resp), AUTH, "disabled"); } @Test @@ -98,9 +119,16 @@ RestResponse resp = getHealthCheckStatus(); resp.assertOK(); - JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); + assertCheckResult(getResponseJson(resp), QUERYCHANGES, "passed"); + } - assertCheckResult(respPayload, QUERYCHANGES, "passed"); + @Test + public void shouldReturnQueryChangesCheckAsDisabled() throws Exception { + disableCheck(QUERYCHANGES); + RestResponse resp = getHealthCheckStatus(); + + resp.assertOK(); + assertCheckResult(getResponseJson(resp), QUERYCHANGES, "disabled"); } @Test @@ -110,9 +138,7 @@ RestResponse resp = getHealthCheckStatus(); resp.assertOK(); - JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); - - assertCheckResult(respPayload, QUERYCHANGES, "passed"); + assertCheckResult(getResponseJson(resp), QUERYCHANGES, "passed"); } private RestResponse getHealthCheckStatus() throws IOException { @@ -125,4 +151,13 @@ assertThat(reviewDbStatus.has("result")).isTrue(); assertThat(reviewDbStatus.get("result").getAsString()).isEqualTo(result); } + + private void disableCheck(String check) throws ConfigInvalidException { + config.fromText(String.format("[healthcheck \"%s\"]\n" + "enabled = false", check)); + } + + private JsonObject getResponseJson(RestResponse resp) throws IOException { + JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); + return respPayload; + } }