Expose styx provider object database in YAML via admin interface #637
Conversation
| .stream() | ||
| .map(entry -> createObjectDef(entry.getKey(), entry.getValue())) | ||
| .collect(Collectors.toList()); | ||
| String output = serialise(objects); |
There was a problem hiding this comment.
An opinionated comment, but I would inline the objects because it is only ever passed on to serialise method. ;-)
There was a problem hiding this comment.
Hm. In this case I find it flows better as it is, because the stream construction is (in my mind) quite different to the method call, and inlining just results in my eye bouncing around a little more than I'd like.
Ideally I'd have liked to be able to follow the .collect() with .serialise() (or some construction with ::serialise) but that's not available. Visually, I think this is the next best thing. But I'll change it (and the others) if you feel moderately strongly about it.
| .build()); | ||
| } | ||
|
|
||
| private Eventual<HttpResponse> handleRequestForOneObject(HttpRequest request, HttpInterceptor.Context context, |
There was a problem hiding this comment.
The request and context arguments are never used.
There was a problem hiding this comment.
Not exactly sure why I left them in.
| .collect(joining("\n")); | ||
| .map(entry -> createObjectDef(entry.getKey(), entry.getValue())) | ||
| .collect(Collectors.toList()); | ||
| String output = serialise(objects); |
There was a problem hiding this comment.
Opinionated: I would inline objects because it is only ever passed, unconditionally, as an argument to objects. Therefore it just adds noise. :-)
| timeoutMillis: 250 | ||
| intervalMillis: 500 | ||
| healthyThreshold: 3 | ||
| unhealthyThreshold: 2 |
There was a problem hiding this comment.
We should add here another object. So that the list would render at least two or more objects.
| tags: [] | ||
| config: | ||
| status: 200 | ||
| content: "Root" |
There was a problem hiding this comment.
Hmm... 🤔
So conceptually this is a map. Also in styx yaml configuration this is represented as a map. But now this implementation renders it as a list.
For consistency, I would prefer the admin interface also to render it as a map.
There was a problem hiding this comment.
Yes, of course. I should have noticed that the config yaml uses maps, not lists here.
There was a problem hiding this comment.
For the case where we are requesting a single object, should it be as now (with name as an attribute) or in a map format (with name as the key)?
There was a problem hiding this comment.
Eh, forget that, it should be the single object payload with no name: attribute and no key.
| } | ||
|
|
||
| scenario("YAML configuration for a single provider is available") { | ||
| val body = styxServer.adminRequest("/admin/providers/objects/myMonitor") |
There was a problem hiding this comment.
A small nag: IMHO the url should be /admin/provider/objects (The provider component should be in singular). /admin/providers/objects sounds wrong because both providers and objects are plural.
Do you think we could change it as a part of this PR, or would this warrant a separate PR?
There was a problem hiding this comment.
There are a number of related URLs that could be changed like this - raised a new issue #641 to cover this.
See #635