TEM-90: add utility external systems check route #225
Conversation
Codecov Report
@@ Coverage Diff @@
## V2 #225 +/- ##
=========================================
+ Coverage 13.26% 13.3% +0.04%
=========================================
Files 41 42 +1
Lines 3975 3979 +4
=========================================
+ Hits 527 529 +2
- Misses 3361 3363 +2
Partials 87 87
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@postables looks like the route got lost in the merge (only the handler is left) |
api/api.go
Outdated
// external checks used by services like load balances | ||
externalChecksProtected := v1.Group("/external-checks") | ||
{ | ||
externalChecksProtected.GET("/systems-check", api.ExternalSystemsCheck) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not:
/external/systems/check
this allows better REST expandability:
/external/api/check
/external/systems/check
/external/systems/list
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even better:
/status/systems/check
/status/systems/...
/status/...
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a lot of the other APIs can be collapsed this way as well, to reduce the number of subroutes and have better grouping, though since that will likely cause compatibility havoc it's not something particularly urgent. I'll open up a ticket tho
@@ -161,6 +161,12 @@ func (api *API) setupRoutes() { | |||
// V1 API | |||
v1 := api.r.Group("/api/v1") | |||
|
|||
// system checks used to verify the integrity of our services | |||
systemChecks := v1.Group("/systems") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to confirm, these APIs should be accessible to anyone right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! It's only intended to give a very basic response. Ie, status 200 and a basic "systems online" as the response body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 馃榿
馃懛 Purpose
Adds a permission-less check used to check basic system status from an external point of view, such as a load balancer, or user wanting to check if the API is online.
馃殌 Changes
Basic 200 OK utility route which requires 0 permissions.
None