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

Fix `RedisClient` instantiation for authentication #785

Merged
merged 1 commit into from Aug 21, 2014

Conversation

Projects
None yet
3 participants
@yamadapc
Contributor

yamadapc commented Aug 19, 2014

This removes the "INFO" command from the default RedisClient
constructor, since its only there to support the redisVersion getter.
This fixes creating clients for databases that require authentication
for all commands. It moves the version fetching logic into the
redisVersion getter, and caches its result in the m_version
property. I think this is the cleanest solution to this issue.

Fix `RedisClient` instantiation for authentication
This removes the "INFO" command from the default `RedisClient`
constructor, since its only there to support the `redisVersion` getter.
This fixes creating clients for databases that require authentication
for all commands. It moves the version fetching logic into the
`redisVersion` getter, and caches its result in the `m_version`
property. I think this is the cleanest solution to this issue.

yamadapc added a commit to yamadapc/dmin.io that referenced this pull request Aug 19, 2014

Get the redis password out of the environment vars
This currently depends on vibe-d/vibe.d#785 and is required
for us to deploy this to Heroku.
@yamadapc

This comment has been minimized.

Show comment
Hide comment
@yamadapc

yamadapc Aug 21, 2014

Contributor

Can I get some feedback on this? It makes deploying vibe.d applications which use redis to heroku impossible. Maybe I missed something, but this is a pretty big bug.

Contributor

yamadapc commented Aug 21, 2014

Can I get some feedback on this? It makes deploying vibe.d applications which use redis to heroku impossible. Maybe I missed something, but this is a pretty big bug.

@yamadapc

This comment has been minimized.

Show comment
Hide comment
@yamadapc

yamadapc Aug 21, 2014

Contributor

I removed a line I shouldn't have, if you want I'll push force and update the commit to be prettier. The ugly diff (because of moving code from a function to another) can't really be avoided. I'm sorry about it too.

Contributor

yamadapc commented Aug 21, 2014

I removed a line I shouldn't have, if you want I'll push force and update the commit to be prettier. The ugly diff (because of moving code from a function to another) can't really be avoided. I'm sorry about it too.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 21, 2014

Contributor

It basically makes m_version lazily loaded. Looks good to me

Contributor

etcimon commented Aug 21, 2014

It basically makes m_version lazily loaded. Looks good to me

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 21, 2014

Member

Looks good indeed. Merging. (Sorry, I completely missed this pull request before)

Member

s-ludwig commented Aug 21, 2014

Looks good indeed. Merging. (Sorry, I completely missed this pull request before)

s-ludwig added a commit that referenced this pull request Aug 21, 2014

Merge pull request #785 from yamadapc/master
Fix `RedisClient` instantiation for authentication

@s-ludwig s-ludwig merged commit d1dd6c9 into vibe-d:master Aug 21, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@yamadapc

This comment has been minimized.

Show comment
Hide comment
@yamadapc

yamadapc Aug 21, 2014

Contributor

Awesome! Thanks :)

Contributor

yamadapc commented Aug 21, 2014

Awesome! Thanks :)

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