Allow setting of Redis connection parameters

[davidwood]

Kue uses the default Redis connection settings and did not provide a mechanism for setting them. I've added the ability to set the parameters that Kue passes to redis.createClient.

lib/redis.js exposes two functions: set which is a chainable function that allows for setting of params and createClient which simply calls redis.createClient, passing any user defined parameters. This new file is named redis.js to minimize integration changes to a the require path.

Both lib/http/routes/index.js and lib/http/routes/json.js create new instances of Queue when required, so I've updated lib/kue.js to require the http module on first access of kue.app, allowing for Redis connection settings to be set first.

[tj]

looks good, thanks for sticking with the style of the project! The one thing I was thinking, since it's always kinda lame when you have to expose ad hoc apis just to tweak settings of some dependency. So I would almost prefer to just use say kue.createClient() and if people want to change things just replace that method. The express app has the same problem, it's really lame to provide an options object, when ideally they can just alter kue.app, but with middleware being append-only it's a bit of an issue right now

[davidwood]

I've used several of your projects and like your coding style, so much so that I've adopted it for my projects, so keeping with the style of the project was second nature :)

I thought about adding doing exactly what you suggested or even adding the set function to lib/kue.js but this would create a circular dependency between lib/kue.js and lib/queue/worker.js, so I figured the safer way was to separate it out.

[tj]

ah :D awesome. Yeah, I don't mind how it is right now. I just find myself always adding APIs to tweak the dep's settings, when it really should just expose the deps in a way that is tweak-able, if that kinda makes sense

[tj]

like the express app, ideally I wouldn't provide any setting like { user: ..., pass: ... } etc. something like this would be the best:

kue.app.prepend(connect.basicAuth(...))

[davidwood]

I really like your suggested approach and wanted to try it out, so I've forked both Connect and Express and added the ability to prepend to the middleware stack. The use function now has the signature use(route, handle, prepend), with prepend being a boolean (that defaults to false). With this new use function, your example can be accomplished with:

kue.app.use(require('connect').basicAuth('username', 'password'), true);

If an explicit prepend function is preferred, a helper could easily be added to app that simple calls use and sets the prepend argument.

If you want to play around with this, check out my forks of Express and Connect. I've also applied this change to the Connect master, which updates proto.js.

[tj]

I think explicit via .prepend or something would be nicer (though it could pass the bool internally), and .use could be an alias of .append or something, I dont find prepend compliments use() well visually, I love use() but maybe it would be worth phasing out

[davidwood]

I think append and prepend are a bit more clean, and for backwards compatibility, they could just be backed by use

[jeromegn]

Would love to have that.

At the same time would be nice to add client.auth() from the redis library.

All the while, I'll be trying to implement it myself within the latest code base.

[davidwood]

If you use my fork, you can add the Redis client.auth by overriding the kue.redis.createClient function. Before you call kue.createQueue, you can do the following:

var createClient = kue.redis.createClient;
kue.redis.createClient = function() {
    var client = createClient();
    client.auth('YOUR_PASSWORD');
    return client;
}

[jeromegn]

@davidwood: Thanks a bunch! Seems to be working, but I'm running into issues. Not sure if it's me or if it's your library hehe. I pulled from the redis-connection-settings branch. So I'm going to try and get it working locally first with the latest kue...

Still hoping for this pull request to go through as soon as it's ready! :)

[davidwood]

@jeromegn What sort of issues? I'm successfully using this approach, so maybe I can help narrow down the cause.

[tj]

something about me really hates adding code to expose options for a dependency, it just seems super hacky, but we definitely need something for now, so i'll merge in a bit

[davidwood]

The alternative to providing a way to set dependency parameters for construction, is to just expose a function that constructs the dependency which can be overridden. This works just as easily and in my example code to @jeromegn yesterday, you'll notice I use that approach to set the auth for the Redis client.

Check out my redis-client-factory branch. redis.js just constructs the Redis client and can easily be overridden to provide custom options.

If there are going to be a bunch of deps in a project, I wonder if it doesn't make more sense to create a generic deps factory (deps.js) that has a bunch of factory methods that can be overridden (e.g. deps.createRedisConnection, deps.createMySQLConnection, etc...)

[tj]

yeah that's typically what I would do, it just gets to the point sometime where it's a lot of duplicated effort just to expose something that exists already

[davidwood]

And generally, a developer who needs to tweak the default settings is capable enough to override a function :) Especially if all that function is doing is constructing and returning an object

[tj]

haha yup! I'd hope so, and less documentation for us

[davidwood]

I updated the readme in my redis-connection-factory branch with an example of overriding redis.createConnection. If it makes it easier for you, I can merge it into the redis-connection-settings branch or open a new pull request with this second branch.

[tj]

yeah i almost think we should just have the createClient which can be overridden

[jeromegn]

@davidwood: Your branch seems to work well, implemented it in production after a while of testing. Does the UI work for you? Doesn't seem to. (as long as it does its job and the API works, it's ok though)

Thanks for the hard work, both of you!

[davidwood]

@jeromegn The UI works for me, but you need to make sure that you set the Redis client parameters prior to accessing kue.app

[tj]

dup :D

[davidwood]

I've removed redis.set and the duplicate export from kue.js I've also updated the readme with a new usage example

[tj]

awesome thanks man, merging

[tj]

damn, it looks like it should apply cleanly but it's not, might have to rebase

[davidwood]

My changes were pretty trivial, let me pull from upstream and merge

[tj]

there we go, git is weird sometimes... complains about merge conflicts and does not leave them for merging haha

[esamattis]

I think this is broken currently. See #54

[davidwood]

See response to issue #54