Skip to content
This repository has been archived by the owner on Jan 9, 2024. It is now read-only.

Add SSLClusterConnection for connecting over TLS/SSL to Redis Cluster #183

Closed
wants to merge 5 commits into from

Conversation

dkent
Copy link
Contributor

@dkent dkent commented Feb 3, 2017

No description provided.

@coveralls
Copy link

coveralls commented Feb 3, 2017

Coverage Status

Coverage decreased (-0.8%) to 86.613% when pulling cc4f513 on outlook:dkent/ssl-cluster-connection into fad035a on Grokzen:unstable.

@Grokzen
Copy link
Owner

Grokzen commented Feb 3, 2017

@dkent Looks good, straight forward and simple.

One comment is to add yourself to authors list in docs/ if you like or not.

I will have to read up on TLS/SSL certs and redis and give this a spin before merging but i see no real problems anyway :) When tested i will merge this.

@Grokzen
Copy link
Owner

Grokzen commented Feb 3, 2017

@dkent All 3.2 redis builds have problems on travis. Will have to wait until fixed before travis-ci passes. All 3.0 tests works tho.

CHANGES Outdated
@@ -0,0 +1,3 @@
Next release
Copy link
Owner

Choose a reason for hiding this comment

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

@dkent Noticed one thing. you should not add this line to this file, you should add it to docs/release-notes.rst instead. Please move it there before i can merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully I made the change correctly. Thanks! :)

@Grokzen
Copy link
Owner

Grokzen commented Feb 6, 2017

@dkent Except from the 1 comment in the code there is only one thing left to do and that is if you want add yourself to authors file. If not then please look at the comment and after that one is fixed i am ready to merge this.

@Grokzen
Copy link
Owner

Grokzen commented Feb 6, 2017

@dkent There is some conflicts that you need to resolve now -_- you probably need to rebase on latest unstable

self.nodes = NodeManager(
startup_nodes,
reinitialize_steps=reinitialize_steps,
skip_full_coverage_check=skip_full_coverage_check,
max_connections=self.max_connections,
**connection_kwargs
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I made a mistake in the review screen. Fixing.

@coveralls
Copy link

coveralls commented Feb 6, 2017

Coverage Status

Coverage decreased (-2.0%) to 83.176% when pulling 84ff9a5 on outlook:dkent/ssl-cluster-connection into b14bb5f on Grokzen:unstable.

@Grokzen
Copy link
Owner

Grokzen commented Feb 6, 2017

@dkent Ye i was pushing merges and changes myself so thats why your stuff broke :) sry for that

@dkent
Copy link
Contributor Author

dkent commented Feb 6, 2017

@Grokzen No worries. I was overly optimistic. I generally fetch and rebase.

@Grokzen
Copy link
Owner

Grokzen commented Feb 8, 2017

@dkent sry but i had to merge this manually becuase there was issues with the merge of unstable you did into you branch. You can find the 2 relevant commits here dabfb97 && 88d38b5

Thanks for taking the time to implement this and submit the patch :)

Closing PR as fixed.

@Grokzen Grokzen closed this Feb 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants