Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Apply ioredis, and add has redis cluster condition #50

Merged
merged 3 commits into from
Apr 25, 2019
Merged

Apply ioredis, and add has redis cluster condition #50

merged 3 commits into from
Apr 25, 2019

Conversation

galaxybing
Copy link

No description provided.

@coveralls
Copy link

coveralls commented Aug 6, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-1.09%) to 98.913% when pulling 1e98f58 on galaxybing:feature-add-redis-cluster into 059476b on koajs:master.

@vampolo
Copy link

vampolo commented Apr 25, 2019

Came across this problem and the fact that redis package does not support clustering as mentioned in redis/node-redis#574 . Any hope for this to be merged?
If not I'll just put this PR in a fork and move along.

@niftylettuce
Copy link
Contributor

I'll take care of this now

@niftylettuce niftylettuce merged commit f551195 into koajs:master Apr 25, 2019
@niftylettuce
Copy link
Contributor

@galaxybing @vampolo could you guys please add docs to README.md to show people how to do cluster support once I release this version?

@vampolo
Copy link

vampolo commented Apr 25, 2019

Thank you very much. The version in this PR is a bit outdated but should do the job. I'll open a PR with a bump in version for ioredis

@niftylettuce
Copy link
Contributor

@vampolo I will bump deps now

niftylettuce added a commit that referenced this pull request Apr 25, 2019
@niftylettuce
fix: bump deps (per #50)
e337a46
@niftylettuce
Copy link
Contributor

@DaAwesomeP @dead-horse could you please grant me NPM owner on GitHub?

npm owner add niftylettuce koa-redis

dead-horse
dead-horse reviewed Apr 26, 2019
View reviewed changes
index.js
@@ -16,6 +16,7 @@
var EventEmitter = require('events').EventEmitter;
var debug = require('debug')('koa-redis');
var redis = require('redis');
var ioredis = require("ioredis");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to switch to ioredis and remove redis, then we can release a major version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take care of this now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dead-horse
Copy link
Member

@niftylettuce added.

@niftylettuce
Copy link
Contributor

I've added the docs now and will be pushing soon for major release

@niftylettuce
Copy link
Contributor

I've just released v4.0.0 which has support for Clusters and Sentinels. Please see updated docs and examples at https://github.com/koajs/koa-redis#koa-redis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dead-horse dead-horse dead-horse left review comments

@niftylettuce niftylettuce niftylettuce left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@galaxybing @coveralls @vampolo @niftylettuce @dead-horse