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

Allows for other common redis options to be in cable.yml, by default #29528

Merged
merged 2 commits into from Jun 27, 2017

Conversation

Projects
None yet
5 participants
@padi
Contributor

padi commented Jun 22, 2017

Summary

Previously, the redis adapter for ActionCable accepts only a :url as an option. While we can follow the redis url scheme, it is not documented well enough in the Rails Guides.

This allows for an alternative setup (that is as common as the previous one) for the redis gem, so people don't occasionally trip up like these. Check the commit message for more technical details.

Other Information

There are other options that can be passed to the Redis gem (See: https://github.com/redis/redis-rb/blob/master/lib/redis/client.rb#L8), but did not include them because:

  1. it might clash with other keys already used in #cable_config in different context (e.g. driver),
  2. the problem of how to test them is non-trivial (at least on top of my head, e.g. path).
  3. they are uncommon enough that they may warrant changing redis_connector on intialization, i.e. ActionCable::SubscriptionAdapter::Redis.redis_connector = ->() { initialization_code }

Also, I'm not as familiar with setting up .travis.yml properly, you may want to check if what I did was right. I removed instances of the Travis' internal redis service. Instead, I used redis-server using the default settings + password in the before_script. The tests now pass though.

Let me know if there's anything else I can do @pixeltrix

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Jun 22, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

rails-bot commented Jun 22, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Allows for other common redis options to be in cable.yml, by default
- Adds RedisAdapterTest::AlternateConfiguration to account
  for a relatively common alternative setup, as it’s used
  as the first example in the
  [Redis rubygem](https://github.com/redis/redis-rb#getting-started)

- Supplies original RedisAdapterTest with more complete
  redis:// url format by adding a ‘userinfo’ (blank user),
  so that it resembles the alternate configuration

- Supplies original EventedRedisAdapterTest with more complete
  redis:// url as well

- Adds before_script to start redis-server with password as a daemon
  and with explicit defaults copied from the default redis.conf
  (Instead of using Travis' default init/upstart scripts for `redis` service)
@padi

This comment has been minimized.

Show comment
Hide comment
@padi
Contributor

padi commented Jun 24, 2017

@rails-bot rails-bot assigned rafaelfranca and unassigned pixeltrix Jun 24, 2017

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 26, 2017

Member

@padi avoid to change the assignment of a PR. We use automatic assigns exactly to avoid to load only one person with all the PRs in the organization (that is a lot). If you assign it again to someone just because you see that person is more active you are overloading that person again.

Member

rafaelfranca commented Jun 26, 2017

@padi avoid to change the assignment of a PR. We use automatic assigns exactly to avoid to load only one person with all the PRs in the organization (that is a lot). If you assign it again to someone just because you see that person is more active you are overloading that person again.

@rafaelfranca

Could you add a CHANGELOG entry?

@benglewis

This comment has been minimized.

Show comment
Hide comment
@benglewis

benglewis Jun 26, 2017

@padi This PR looks to fulfill the needs that I had in my pull request. Can I add the CHANGELOG for you?

benglewis commented Jun 26, 2017

@padi This PR looks to fulfill the needs that I had in my pull request. Can I add the CHANGELOG for you?

@padi

This comment has been minimized.

Show comment
Hide comment
@padi

padi Jun 27, 2017

Contributor

@rafaelfranca genuinely sorry about that. I didn't think that one through. ☕️ for you should we get to meet in any of the ruby conferences.

@ukblewis glad to help. 👍

Contributor

padi commented Jun 27, 2017

@rafaelfranca genuinely sorry about that. I didn't think that one through. ☕️ for you should we get to meet in any of the ruby conferences.

@ukblewis glad to help. 👍

@rafaelfranca rafaelfranca merged commit 1eaed91 into rails:master Jun 27, 2017

1 check passed

codeclimate no new or fixed issues
Details
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2017

Member

Thank you!

Member

rafaelfranca commented Jun 27, 2017

Thank you!

@padi padi deleted the padi:actioncable_redis_alt_config_with_password branch Jun 27, 2017

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