Code review comment for lp:~denys.duchier/bzr/bzr.ssl

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> "Denys" == Denys Duchier <email address hidden> writes:

    Denys> Ah yes! I had not noticed ssl_certs. This is
    Denys> perfect; I'll use it instead of my hack. Did you have
    Denys> more than this reuse in mind?

_ssl_wrap_socket in bzrlib/transport/http/_urllib2_wrappers.py
looks very much like your wrap_socket :-)

I'm pretty sure the expand* should go somewhere else, closer to
the CLI UI... i.e. higher in the stack.

Then, since this is now needed in two different places, that may
need to be moved to osutils.py instead.

Other than that, the way you reuse the tests in
blackbox/test_server.py is not exactly what we prefer :)

You want to move these tests in their own class and make them
parameterized instead.

So I'm voting resubmit (the points above are worth doing a new
submission with them fixed but the overall patch sounds good) and
with that sorted out, I'd leave it to Andrew to review the smart
server part of it.

 review: resubmit

 reviewer: spiv

review: Needs Resubmitting

« Back to merge proposal