Code review comment for ~slingamn/ssh-import-id:norequests.release

Revision history for this message
Robie Basak (racb) wrote :

Thank you for working on improvements to ssh-import-id!

Note that cloud-init depends on python3-requests. Are you looking to remove the requests dependency from there, too? Without that, I don't see how this change will help you towards your goal.

However, now that the standard library has sufficient support, I have no objection to removing the dependency on requests from ssh-import-id. Unless it makes the code worse, preferring the standard library over an external library is usually a good thing.

I'm not sure about the shim you've added to emulate enough of the requests library here though. What I'd expect to see instead is ssh-import-id adjusted to use the standard library as if it was written to do that in the first place. But instead there's now this extra indirection through your new shim, including some protocol-specific handling that looks like it should be in a dedicated protocol handling library rather than reimplemented inside a high level tool like this one.

What's the reason you chose to use http.client instead of urllib.request? Then you could avoid the URL inspection in http_get() I think? Is there some feature needed that is missing from urllib.request?

If I'm wrong or I've missed something, please could you explain? Otherwise, please drop the new https.py and change the URL fetch code in the two places it is used to natively use urllib.request instead, handling the resulting HTTPResponse object directly rather than emulating the old response object that you want to remove. You can use urllib.request.Request() to include User-Agent to maintain the existing functionality. Feel free to abstract common code out to a function if appropriate.

review: Needs Fixing

« Back to merge proposal