-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(admin) remove DB counters in favor of a connection test #2567
Conversation
assert.is_number(json.server.connections_writing) | ||
assert.is_number(json.server.connections_waiting) | ||
assert.is_number(json.server.total_requests) | ||
assert.True(json.database.reachable) |
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.
style: assert.is_true()
716e4fb
to
a564e48
Compare
Ping @shashiranjan84 @p0pr0ck5 @thefosk |
@thibaultcha LGTM |
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, one nonblocking question
it("obfuscates sensitive settings from the configuration", function() | ||
|
||
it("database.reachable is `true` when DB connection is healthy", function() | ||
-- In this test, we know our DB is reachable because it must be |
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.
not a blocker for this PR, but could we simply mock out the dao's reachable
function to test this?
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.
Possibly, although those are "integration" tests, and supposed to be through-to-through. Mocking it is still slightly better, but won't test the reachable method properly. Also, we don't have a process to inject stubs into a running Kong started by the integration tests... 🤔
Change added to the wiki's Changelog as well |
This is a replacement proposal for #2554. As discussed, the point of this change is to provide a connection test to the database, and not to evaluate its health. Following this logic, this patch is more aligned with the intended goal.
cc @shashiranjan84 @p0pr0ck5 @thefosk