Skip to content

Conversation

momer
Copy link
Contributor

@momer momer commented May 14, 2014

This is an expansion of @denibertovic 's work on #187

I'd re-done some of his work today, and expanded on it quite a bit by the time I realized it existed here. I've forked his commit to give him credit, and expanded on his work.

Note: This is my first time writing Python longer than 5-10 lines. Let me know if there are any code smells! I know the Exception class can be a sticking point, but I felt it was important to remind the user where the TLS API comes from, and to DRY that up a bit.

Also, I left a bunch of comments to help readers understand why some of the additions are necessary. Tests pass, and I connect fine using the options; but, alas, I haven't written any new tests for this PR.

denibertovic and others added 2 commits March 22, 2014 14:09
…t for SSL in docker-py. Including the ability to specify the expected SSL Version for issues with OpenSSL sslv3/tls1 recognition issues. Added an exception class for repetitive reminders to look at the CLI doc on docker.io.
@momer
Copy link
Contributor Author

momer commented May 15, 2014

Thoughts?

@shin-
Copy link
Contributor

shin- commented May 21, 2014

Thanks!

Ok, so you'll want to rebase this first, especially because we already have an exceptions errors module in master which you'll want to merge with your own.

Otherwise this looks good. Maybe extract all the additional checks and configs from __init__() and into a separate method so it doesn't become too crowded?

@momer
Copy link
Contributor Author

momer commented May 21, 2014

Sounds good - will get to this tonight/later this week

@shin-
Copy link
Contributor

shin- commented Jun 21, 2014

@momer Are you still working on this?

@momer
Copy link
Contributor Author

momer commented Jun 21, 2014

@shin- sorry for the delay had a hectic deadline to meet. Changes in docker-py have since made my updates break tests in master. Would love it if you could pick up here and patch forward to current version. I may have time in the next month to get to this, but it's low on my priority unfortunately.

@shin-
Copy link
Contributor

shin- commented Jun 23, 2014

No worries, thanks for the quick reply.

@shin- shin- mentioned this pull request Jun 23, 2014
@shin-
Copy link
Contributor

shin- commented Jul 2, 2014

I'm closing this since we're continuing the discussion on #250 :)

@shin- shin- closed this Jul 2, 2014
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