Make RedisConnection Check Require Redis Instance Parameter #39
Conversation
kylesnowschwartz
requested review from
travo,
petervandoros,
lamorrison and
BlakeJohnCarter
kylesnowschwartz
force-pushed
the
add_instance_parameters_to_redis_check
branch
2 times, most recently
from
70e718b
to
b1aa3bb
Compare
| class RedisConnection | ||
| attr_reader :instance_parameters |
There was a problem hiding this comment.
Consider renaming this to something more accurate. E.g., redis_instance?
There was a problem hiding this comment.
The sequel check uses connection_parameters to indicate that it takes the necessary parameters to construct a sequel database instance — I'm wondering if we should take the same approach here.
There was a problem hiding this comment.
Given the possible complexity with redis connection handling (e.g., connection pooling), I'm thinking it's probably best to pass the redis instance this check should use. Leaving that complexity to the app keeps this check simpler.
There was a problem hiding this comment.
I've gone ahead and renamed to redis_instance which conveys what is being passed in via the parameters hash more clearly.
|
Sorry to make extra work for you @kylesnowschwartz, but I've just merged my other long-standing PR that includes changes to the README file — do you mind editing the README to reflect the configuration changes as part of this PR? |
Redis-rb, the Redis gem used by Rails apps, is deprecating a part of its API which stores a global Redis connection. See redis/redis-rb#1064 for context around that deprecation. To prepare for when this behavior will be removed, we must update this application so that Redis will be passed in as an instance to be checked, because it will not be globally available.
Simpler name conveys what this variable is
kylesnowschwartz
force-pushed
the
add_instance_parameters_to_redis_check
branch
from
b1aa3bb
to
4728499
Compare
|
@petervandoros does this require pumping the version from 0.1.0 to 0.2.0 because of the breaking API change? |
|
@kylesnowschwartz I'll handle bumping the version, as I have one more change I want to include for the next release (deprecating the built-in |
Update API documentation with correct types Co-authored-by: Liam Dawson <liam.dawson@envato.com>
Redis-rb, the Redis gem used by Rails apps, is deprecating a part of its
API which stores a global Redis connection. See
redis/redis-rb#1064 for context around that
deprecation.
To prepare for when this behavior will be removed, we must update this
application so that Redis will be passed in as an instance to be
checked, because it will not be globally available.
See https://github.com/envato/marketplace/pull/29383 for more information