Switch away from 'requests' and standardize SCMTool HTTP usage.

Review Request #2901 — Created Feb. 19, 2012 and submitted

Information

Review Board
release-1.6.x

Reviewers

Switch away from 'requests' and standardize SCMTool HTTP usage.

The 'requests' module, while nice on the outside, does not meet our
needs today. It doesn't support Python 2.4 or 2.5 (which RB 1.6.x does
support), and it's unclear what support will be going forward.

In reality, the main reason we're using it in 1.6.x at all is because we
needed to pre-populate the Authorization header, which requests will do
automatically. However, this isn't anything special. We can just add
that header ourselves and keep urllib2, which is fine because our needs
there are very basic.

In order to prevent code duplication, I've created a SCMClient class
that Git's and Mercurial's clients inherit from. It provides at the
moment one utility function, get_file_http, which wraps the urllib2
logic. If we switch back at some point to requests or another module, we
need only update this function. We probably won't need to, though.

In the end, both classes are simplified, we don't need this dependency,
we keep our Python support, and pre-populated authorizations work.
Tested this with a couple different Git and Mercurial repos, with and without
HTTP Basic Auth.

In particular, I made sure this still worked with CodebaseHQ's Git repository.

I also confirmed with Wireshark that the first request had the Authorization
header.
Description From Last Updated

Can you make this more verbose? If it doesn't include the source URL, just looking at the "error" log will …

daviddavid

Should we just be replacing https with http? If https has been specified, the user might have an expectation of …

SM smacleod
SM
  1. Everything looked good to me, just one little issue.
  2. reviewboard/scmtools/hg.py (Diff revision 1)
     
     
    Show all issues
    Should we just be replacing https with http? If https has been specified, the user might have an expectation of encryption which is not being provided.
  3. 
      
david
  1. Well, I'm sad that it turned out to be a dud. I <3 the API :(
    
    This change looks pretty good. Just one comment.
    1. Yeah me too.. We can maybe look into it again in the future once we know more about their Python compatibility schedules (looks like they're moving to Python 3 faster than we can with Django), and if we feel comfortable with their codebase and stability.
  2. reviewboard/scmtools/core.py (Diff revision 1)
     
     
    Show all issues
    Can you make this more verbose? If it doesn't include the source URL, just looking at the "error" log will be useless.
    1. Doh, that was for my own testing. I'll fix that :)
  3. 
      
chipx86
Review request changed

Status: Closed (submitted)

Loading...