Re-register provider endpoints with admin interface after updates #572
Conversation
There was a problem hiding this comment.
To sum up:
-
Check my comments about Thread.sleep() in tests. This is why I'm setting the PR to Request Changes state.
-
Since this implemented feature is not specific to health check services alone, consider testing this with a mock service implementation. You could inject one via StyxServerComponents IIRC. This would avoid having to create mock origins and health checking them just for testing dynamic changes to admin interface providers.
-
Consider tweaking the URL router as per comments.
-
As a team we should consider to write all new code in Kotlin instead of Java. Now we are having a mixture of both. But this is a separate topic.
| String providerName = entry.getKey(); | ||
| entry.getValue().getStyxService().adminInterfaceHandlers(providerPath(providerName)) | ||
| .forEach((relPath, handler) -> | ||
| routeBuilder.get(endpointPath(providerName, relPath), new HttpStreamer(MEGABYTE, handler)) |
There was a problem hiding this comment.
We could simplify this code a bit by tweaking the UrlpatternRouter. So if UrlPatternRouter took a path prefix constructor argument, in this case /admin/providers, then all the subsequent calls to routeBuilder.get could simply pass in the relative path.
I think that would simplify this code a bit and we could possibly get rid of providerPath and endpointPath methods.
| @@ -21,6 +21,7 @@ import io.kotlintest.Spec | |||
| import io.kotlintest.matchers.string.shouldContain | |||
| import io.kotlintest.matchers.string.shouldMatch | |||
| import io.kotlintest.specs.FeatureSpec | |||
| import java.io.File | |||
| @@ -64,6 +65,7 @@ class AdminInterfaceSpec : FeatureSpec() { | |||
| .shouldMatch("\"[0-9]{1,2}d [0-9]{1,2}h [0-9]{1,2}m\"".toRegex()) | |||
| } | |||
| } | |||
|
|
|||
| Thread.sleep(5000) | ||
| val response = styxServer.adminRequest("/admin/providers/appA-monitor/status") | ||
| response.status() shouldBe OK | ||
| response.header(HttpHeaderNames.CONTENT_TYPE).get().toLowerCase() shouldBe HttpHeaderValues.APPLICATION_JSON.toString().toLowerCase() |
There was a problem hiding this comment.
Use static Imports for HTTP header names.
| } | ||
|
|
||
| scenario("The new endpoint returns status information after server restart") { | ||
| Thread.sleep(5000) |
There was a problem hiding this comment.
Why the sleep here? Also 5 seconds is quite long. Is this justified?
There was a problem hiding this comment.
No, it's not needed at all. It was left over from investigating something earlier in development.
| scenario("The new endpoint returns status information without server restart") { | ||
| val response = styxServer.adminRequest("/admin/providers/appB-monitor/status") | ||
| response.status() shouldBe OK | ||
| response.header(HttpHeaderNames.CONTENT_TYPE).get().toLowerCase() shouldBe HttpHeaderValues.APPLICATION_JSON.toString().toLowerCase() |
…5-healthchecker-registration
…tional test that used it.
| scenario("Additional endpoints are listed on the Providers admin interface") { | ||
|
|
||
| // styxServer.components().servicesDatabase().insert("mockService", | ||
| // ProviderObjectRecord("MockService", setOf(), mockk(), MockService("mockService"))) |
There was a problem hiding this comment.
Some unnecessary comments here.
|
|
||
| scenario("Endpoints for dynamically removed Styx services are not listed in the Admin interface") { | ||
|
|
||
| // styxServer.components().servicesDatabase().remove("mockService") |
See #555