-
Notifications
You must be signed in to change notification settings - Fork 26
Store checks results on the db instead of ARA #415
Conversation
27d5f37
to
4a13617
Compare
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.
Thanks for this! LGTM except minor cosmetical issues. Feel free to sort them out in a different PR if you agree with them (or here if you prefer)
// @Router /api/clusters/{cluster_id}/results [get] | ||
func ApiClusterCheckResultsHandler(client consul.Client, s services.ChecksService) gin.HandlerFunc { | ||
// @Router /api/checks/{id}/results [post] | ||
func ApiCreateChecksResultstaHandler(s services.ChecksService) gin.HandlerFunc { |
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.
The name of the function seems to have a typo: ApiCreateChecksResultstaHandler
-> ApiCreateChecksResultsHandler
mockChecksService := new(services.MockChecksService) | ||
mockChecksService.On( | ||
"GetChecksResultAndMetadataByCluster", "47d1190ffb4f781974c8356d7f863b03").Return( | ||
&models.ClusterCheckResults{}, fmt.Errorf("kaboom")) | ||
"GetChecksResultAndMetadataById", "47d1190ffb4f781974c8356d7f863b03").Return( |
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've noticed that sometimes you use "CheckResults" and others you move the plural: "ChecksResult". Is this intentional?
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.
Also sometimes I see "ChecksResults". There might be a reason I'm missing but if not, I'd try to keep it constant across the code.
assert.Equal(t, 404, resp.Code) | ||
} | ||
|
||
func TestApiCreateChecksResultstaHandler(t *testing.T) { |
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.
Also here ofc :-)
GetChecksResultAndMetadataByCluster(clusterId string) (*models.ClusterCheckResults, error) | ||
GetAggregatedChecksResultByHost(clusterId string) (map[string]*AggregatedCheckData, error) | ||
GetAggregatedChecksResultByCluster(clusterId string) (*AggregatedCheckData, error) | ||
GetChecksResultById(id string) (*models.Results, error) |
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.
Basically the same as already commented
To unblock this, for the time being, let's use the public api Gin instance, which was a quandary left unsolved since the block. |
Closing this as the conflict resolution looks more difficult than opening a new fresh PR. |
Store the checks results content in the database instead of ARA.
The major changes:
CheckResultsRaw
(check_results
is the actual name on the database). This table contains the checksGroupID
(which by now is always the cluster id) and the results json as payload. It contains the creation time and an unique ID as well.There are a lot of ARA leftovers in the code yet. I will remove them definitely once this PR is done, as this pieces of code are the ones that really use ARA by now.
PD: I still used the
JSON
prefix to define the API structs. We already have a follow up ticket at: #412