redis-rb / redis-client

Simple low level client for Redis 6+
MIT License
124 stars 60 forks source link

adds url as optional kwarg for sentinel config #117

Closed moofkit closed 1 year ago

moofkit commented 1 year ago

Description

It's a proposal PR to adds url option for client config with sentinels After updating redis gem to 5.0 there are differences in sentinel configuration.

Also it is a little confusing for users of redis-rb and sidekiq

Before update it was possible to configurate name, username, password, db via url without explicit setting:

Redis.new(
   url: 'redis://user:pass@mymaster/12', 
   sentinels: [
      { host: "127.0.0.1", port: 26380 },
      { host: "127.0.0.1", port: 26381 },
    ]
)

This config will provide client connection to redis host with username user, password pass and database 12.

In the newest version it has to provide connection options explicitly:

Redis.new(
   name: 'mymaster',
   db: 12,
   username: 'user',
   password: 'pass',
   sentinels: [
      { host: "127.0.0.1", port: 26380 },
      { host: "127.0.0.1", port: 26381 },
    ]
)

The problem is that it's not very handy to provide a bunch of variables like REDIS_USERNAME, REDIS_DB, REDIS_PASSWORD, but have one plain REDIS_URL to configure connection. So to stick with url it needs to write custom code to parse url something like that:

redis_url = ENV.fetch("REDIS_URL", "redis://user:pass@mymaster/10").to_s
uri = URI.parse(redis_url)
host = uri.host
user = uri.user
pw = uri.password
db = uri.path.split("/").last.to_i

redis_config = {
                   db: db,
                   name: host,
                   sentinels: JSON.parse(ENV.fetch("REDIS_SENTINELS", "[]")).map(&:symbolize_keys),
                   username: user,
                   password: pw
                 }
redis = Redis.new(**redis_config)

So this PR is proposal to return old behavior to give an option to provide url to config as alternative to explicit kwargs

casperisfine commented 1 year ago

So I'm ok on principle, but I have a few concerns about the implementation, most notably the concept of doing parser.to_h.slice(...).

I think it would be better to just explicitly deal with the defaults in both initializers.

moofkit commented 1 year ago

@casperisfine thanks for review and your suggestions!

I've refactored it in favour of explicit approach. Check the changes out please

casperisfine commented 1 year ago

Merged as https://github.com/redis-rb/redis-client/commit/ed821c0542eff31e227776f904c6a9c60a590f59 (I did some small edits)