-
Notifications
You must be signed in to change notification settings - Fork 26
Add check selection api to the server #357
Add check selection api to the server #357
Conversation
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.
A tiny comment, but I think we are ready to merge after this gets addressed or clarified
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.
LGTM
de00d88
to
db9a1db
Compare
@dottorblaster @fabriziosestito I have updated the PD: I have been forced to create the new |
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.
@arbulu89 LGTM, thanks!
753a389
to
8b28a10
Compare
8b28a10
to
3e52da6
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.
Looks quite good to me, good job @arbulu89!
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.
Good stuff!
My OCD would prefer a slightly different setup for tests. But that's not a blocking. 🚀
t.Fatal(err) | ||
} | ||
|
||
// 200 scenario |
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 would probably refactor to use testsuites and maybe split Scenarios into smaller test functions, but this just works as fine! 💪
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.
Yes, initially I thought the same. But as it already had pretty complete tests (test 200, 404, 500), I just followed the same approach.
Nothing against using suites or smaller functions :D
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.
LGTM, we might start thinking of dividing api in files or subpackages as it is growing a lot.
Agree! I would create an |
This PRs implements one of the 1st components to remove consul usage from checks service.
It stores the selected checks in the new Postgres database, and offers the API endpoints to do so externally. This 2nd things will be needed from the runner, as it needs to get the currently selected check ids. The created service code will be used in the
cluster
web handler as well, to remove the current consul code.The
SelectedChecks
information will go in a new table with the next struct:ID
, the selected resource ID. By now, this will be the cluster id, but it will scale if we add other concepts checksSelectedChecks
, a comma separated list of checks as a string. This copies the current exact type. We can store it as an array if needed, I just migrated the current type for easiness sakeI have more or less the subsequent code ready (adapt the runner and server sides), but I preferred to keep the PR small to improve the review process