wistia / nsq-ruby

NSQ Ruby client
MIT License
68 stars 27 forks source link

Simple TLSv1, take 2 #26

Closed freerobby closed 7 years ago

freerobby commented 7 years ago

This builds on @alieander's work in https://github.com/wistia/nsq-ruby/pull/25, adding some specs and building toward the interface we agreed on at the end of that PR. Please continue discussion in this PR; I will close the other one.

ghost commented 7 years ago

@freerobby, thanks for your PR! By analyzing the annotation information on this pull request, we identified @alieander, @bschwartz and @mmoutenot to be potential reviewers

bschwartz commented 7 years ago

Given that this is a breaking change from the current version (in that folks who have code that sets ssl_context will no longer connect via TLS, since tls_v1 defaults to false), should we also change ssl_context to be called tls_options to make it more consistent with other client libraries?

Also, mind adding the start of a CHANGELOG? Figure that will be helpful for folks who will upgrade.

mschneider82 commented 7 years ago

I would keep the ssl_context value to make sure the warn is triggered if there is a existing client certificate validation configuration. The boolean tls_v1 matches the nsq tcp specs

freerobby commented 7 years ago

@bschwartz I created a changelog and renamed ssl_context to tls_options.

@mschneider82 We borrowed the tls_options name from pynsq. But to mitigate the point you rase, I do look for ssl_context and use it if tls_options is not provided, so this should catch both cases. Applicable tests are also now run against both usages.

Thanks everyone! I realize this ballooned a bit but think we got it to a good place. Let me know if you have any more suggestions, else I'll merge and release tomorrow.

alieander commented 7 years ago

Looks great! Thanks for taking the time to sort everything out!

bschwartz commented 7 years ago

@freerobby Nice! Just left some comments on the README. Code looks great. Thanks for sorting all this out -- I think it all makes way more sense and the naming is good.

freerobby commented 7 years ago

@bschwartz Thanks, agree with all your comments, just pushed changes accordingly.

bschwartz commented 7 years ago

Nice! 🚀!

freerobby commented 7 years ago

Merged and released. Thank you @alieander for adding this!