ledgetech / lua-resty-redis-connector

Connection utilities for lua-resty-redis
234 stars 71 forks source link

Fixes around creating new connections #34

Closed piotrp closed 4 years ago

piotrp commented 4 years ago

A few fixes separated into a few commits:

pintsized commented 4 years ago

Hey, thanks for this. Seems these changes break the tests though, could you try running them on your side? You'll need to make start_redis_instances, followed by make test.

Here's what I'm getting: https://gist.github.com/pintsized/bff6c6beaa4799762788a32fcd1a8884

piotrp commented 4 years ago

Oops... sorry for that, fixed.

There were two issues, both in my first commit:

  1. I removed logging of an error message when AUTH command failed, and there was a test that looked for it.
  2. My original fix to error returned from :connect didn't account for Sentinels: :connect tries to set database, which is an unsupported operation in Redis Sentinel, and possibly that's why error handling of that call was originally omitted.
piotrp commented 4 years ago

Hello, @pintsized, were you able to look into this MR?

pintsized commented 4 years ago

Hey, I'm really sorry I seem to have totally forgotten about this. I managed to get travis CI working on this repo, if you rebase it should pick that up and build your pr.

Otherwise, yep LGTM. Looks like there's a 2-space indentation on line 363 if you feel like making it perfect ;)

piotrp commented 4 years ago

Invalid formatting is a perfectly valid issue, rebased and fixed :)