-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add SSL option for redis client #5
base: master
Are you sure you want to change the base?
Conversation
@tillepille I'm actually not involved in this redis plugin. your changes look good though. maybe @masom has a spare minute to look at this? 👋 |
@tillepille i'm not sure how many active maintainers there are for the thumbor-community plugins. would you be interested in helping out? |
@phoet I think I'm not the best choice, since I'm not a python enthusiast(,yet) nor someone who writes code a lot... |
Ditto, though I changed jobs so I'm not dealing with python or using thumbor any longer... @masom is probably busy at Shopify at the moment. I'll try to reach out to him if he does not react to this within the next couple of days. I think there was some slack-group for this, but I can't find it right now... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing this patch to tc_redis.
Would it be possible to add a test in https://github.com/thumbor-community/redis/blob/master/vows/redis_storage_vows.py to ensure the new option is correctly working?
@masom Since this is a flag which is present in the underlying lib, do you want to test the functionality of that lib? |
I think testing the functionality of the lib is a tiny bit out of scope although we should ensure we are indeed passing valid options to the lib (so it doesn't raise on init). The test itself could just be as simple as connecting with SSL enabled, and checking that we are indeed connected with SSL. Looking at the redis-py docs, there's also a few more SSL options, should those also be implemented? |
@masom |
closes #4