Skip to content

Conversation

@2rba
Copy link

@2rba 2rba commented Feb 23, 2021

WHY

Secure redis connection might require additional connection params

WHAT

  • Added Redis options ENV variable
  • Added ENV variables description
  • Added CircleCI example

Copy link

@alexfilatov alexfilatov left a comment

Choose a reason for hiding this comment

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

LGTM

@fragoulis
Copy link
Contributor

fragoulis commented Mar 31, 2021

Hi and thank you for the time you took to make this. We have reviewed the PR at the time it was opened but we were overwhelmed with work (and still are).

Firstly, I agree with being able to support your use case. I have to say, however, I haven't seen this pattern of passing arbitrary options thourgh json to a cli. Can you provide of some examples of some other established cli tools that follow this pattern.

I would probably go with supporting a config file of some sort for more complex configuration. We also try to abide by some rules around the cli tool.

EDIT:

After seeing your addition to the README I updated the master with the current env variables. Thank you for that idea.

@2rba
Copy link
Author

2rba commented Mar 31, 2021

@jfragoulis Thank you for your comment.
I don't like JSON options as well. There reason for it ssl_params might contain quite different options.
From https://github.com/redis/redis-rb

:ssl_params => {
    :ca_file => "/path/to/ca.crt",
    :cert    => OpenSSL::X509::Certificate.new(File.read("client.crt")),
    :key     => OpenSSL::PKey::RSA.new(File.read("client.key"))
  }

And each environment might require different options. An attempt to define them all as command-line arguments might overload command-line options with noisy params.

I was thinking about what could be the other option,
Could redis connection be an injectable config option?
like Redisq.config.redis_connection
this way if someone needs a hand-tailored Redis connection it can be injected somewhere in rails_helper.rb as

Redisq.config.redis_connection = Redis.new

What do you think?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants