-
Notifications
You must be signed in to change notification settings - Fork 189
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
Fixes #1938: Updated healthCheck for Posts service #2165
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.
One small thing and this is ready.
Reading your code, I also realize that we need to add a way to tell the Redis connection to close just as we're shutting down. Right now we don't expose a hook for this from Satellite, but we can add one.
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 think to get the unit tests to pass, you're going to have to move your redis
instance out of index.js
and into its own file? Not sure, but redis
being undefined in the tests is odd.
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 besides the nit pointed out by Dave.
oops, didn't notice failing checks, will re-evaluate after fix + rebase
@humphd. I might need to wait for the shutdown option to be merged into Satellite. Even in it's own file, redis seems to be undefined which is super weird |
I had an epiphany while on a walk today: did we add Redis since the last npm update for Satellite? We probably need to ship an update that includes it. I just triggered 1.12.0, which should hit npm within the next 30 mins. Let's try that and see what happens here. |
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.
This looks good. The only minor nit I have is that you're only exporting a single thing from redis.js
, so I would module.exports = redis
instead, so you can const redis = require('./redis');
and not bother with destructuring. But it's minor.
I agree, we should do a follow-up to add the shutdown logic for redis too, once that lands.
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.
Perfect.
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
Rebased, @humphd @chrispinkney, let's get this merged in |
Issue This PR Addresses
This PR closes #1938
Type of Change
Description
With Redis added to Satellite, I'm able to update the posts service's health check to check if redis is functioning by using the ping function from Redis. This PR also updates the routes for the posts service to use the createError function and call that function using the
next
function as part of the catch.Checklist