Allow automatic SSH key association when adding/editing GitHub repositories

Review Request #3423 — Created Oct. 16, 2012 and submitted

Information

Review Board
master

Reviewers

Defines an interface to allow a Review Board instance's SSH key to be added to
a hosting service's list of deploy keys for a given repository. So far, only 
the GitHub HostingService implements the necessary methods to upload the SSH
key, and raises an exception when the upload fails. The code is not currently
being used by any module/app other than tests.
Unit test for GitHub auto key association passes. Tested that key is delivered
and added to the deploy keys of a GitHub project.
Description From Last Updated

Add a trailing comma.

chipx86chipx86

Remove this blank line.

chipx86chipx86

We should log what we get in this case. Both the HTTP exception data and the exception when loading/reading.

chipx86chipx86

If we're going to log something, we need to have some more context, so it's clear when reading the log …

chipx86chipx86

Must pass the local_site tied to the repository.

chipx86chipx86

We don't use the "if else " form.

chipx86chipx86

Make sure the setUp and tearDown functions in this class back up and restore these functions.

chipx86chipx86

Add a trailing comma.

chipx86chipx86

Two blank lines.

chipx86chipx86

Align SSHKeyAssociationError with AuthorizationError.

chipx86chipx86

You need to return something if there is no key. Actually, is this even called without a key? If not, …

chipx86chipx86

We should just always raise this. Nothing should ever call this without checking, so it's a bug either way.

chipx86chipx86
KA
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Should this return the key that's been associated (probably as a string), so it can be stored in the db as Repository field?
    
    A general version of this method will be added to the base class.
    1. I think this function should actually take a key, rather than assume get_user_key. Then it doesn't need to return it, but the caller can be specific about what's being associated.
  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    This will probably make it into the base class. Is there anywhere else this formatting might be useful?
    1. Let's keep it here until we know we need this exact data elsewhere.
  4. 
      
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    Add a trailing comma.
  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
    Show all issues
    Remove this blank line.
  4. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    We should log what we get in this case. Both the HTTP exception data and the exception when loading/reading.
  5. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    If we're going to log something, we need to have some more context, so it's clear when reading the log file.
    1. Went with raising an exception only (no logging) -- is it worth it to do both here?
  6. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    Must pass the local_site tied to the repository.
  7. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    We don't use the "if <X> else <Y>" form.
  8. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    Make sure the setUp and tearDown functions in this class back up and restore these functions.
  9. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Show all issues
    Add a trailing comma.
  10. 
      
KA
chipx86
  1. Heads up: The new SSH code has been committed to release-1.6.x and master.
  2. 
      
KA
chipx86
  1. This is looking pretty good. Are you about ready to take it out of "WIP" status? (We won't commit if it says "WIP", as we'll assume you don't want us to yet.)
  2. reviewboard/hostingsvcs/errors.py (Diff revision 3)
     
     
     
     
    Show all issues
    Two blank lines.
  3. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
     
    Show all issues
    Align SSHKeyAssociationError with AuthorizationError.
  4. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
     
    Show all issues
    You need to return something if there is no key.
    
    Actually, is this even called without a key? If not, don't bother to check.
  5. reviewboard/hostingsvcs/service.py (Diff revision 3)
     
     
     
    Show all issues
    We should just always raise this. Nothing should ever call this without checking, so it's a bug either way.
  6. 
      
KA
KA
chipx86
  1. Ship It!
  2. 
      
KA
Review request changed
Status:
Completed
Change Summary:
Pushed to master (afc96e2)