HostingServices can check if SSH keys are associated

Review Request #3478 - Created Nov. 7, 2012 and submitted

Karl Leuschen
Review Board
master
reviewboard
Automatic SSH key association should only be performed if the key has not
already been associated. HostingServices that support SSH key association
now have the capability to report if a given key has been associated with a
given repository. Currently, only the GitHub HostingService supports SSH
key association.
Added unit test for code that checks if a key has been associated. Made slight adjustment to 
key association test to ensure tests pass (given that a helper method for SSH key association
API calls was added). Both unit tests pass.

Tested manually with an existing GitHub repository by:
    1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed 
        key was associated.
    2) adding SSH key to list of user keys. Check confirmed key was associated.
    3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
    4) ensured associate_ssh_key still works as normal for GitHub.
  • 0
  • 0
  • 16
  • 6
  • 22
Description From Last Updated
Christian Hammond
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
    No blank line.
  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
    Instead of formatting it this way, do:
    
    ('%suser/keys?access_token=%s'
     % (self.API_URL, ....))
  4. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
     
    It'd be nice to change the code to, at the best case, only check one batch of keys. So, get one set, check them. If not found, get the other set, check them.
    
    Not sure which set to check first. Guess if we're eventually going to upload deploy keys, we should do that.
    1. Agreed. Deploy keys will probably be more likely, especially if this feature gets used.
  5. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
    this can just be:
    
    return formatted_key in keys
  6. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
    Why are we factoring this out? Will more things be calling this?
    1. No -- only is_ssh_key_associated and associate_ssh_key use it. It might be overkill, but it cleaned up the code a little. It's no trouble to scratch it, so just let me know.
  7. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
    Missing period.
  8. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
    ``key``
  9. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
    ``repository``
  10. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    One dict per line.
  11. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Let's precompute this once outside the function.
  12. 
      
Karl Leuschen
David Trowbridge
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
     
    Can you align the "if" with the "for" in the previous line?
  3. reviewboard/hostingsvcs/service.py (Diff revision 2)
     
     
    Why did you change the single backticks to be double backticks?
    1. Christian mentioned the change as part of his feedback on r1. I assumed it was because Sphinx uses reStructuredText for formatting, and I had written the docstring with my brain running in Markdown.
    2. Cool, just wondering.
  4. 
      
Karl Leuschen
Christian Hammond
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Just so the entire function doesn't have to be indented, it'd be nice to just do:
    
    if not key:
        return False
    
    ... # rest of code
  3. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
     
    I know this has gone back and forth a couple times, but really, multi-line like this should be:
    
    deploy_keys = [
        item['key']
        for ...
        if ...
    ]
    
    Same below.
  4. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     
     
    Space after :
  5. 
      
Karl Leuschen
John Sintal
  1. 
      
    1. Most of the issues you pointed out are on lines this patch doesn't modify, so I'll do a pass on the hostingsvcs app in another review request to clean up the PEP8 violations. Thanks for the heads up!
  2. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Continuation line indentation is not a multiple of four
    1. This is shows up occasionally in the RB codebase -- this particular alignment is (slightly) more readable and trumps the PEP8 complaint in this case.
  3. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    line too long (80 > 79 characters)
  4. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    continuation line does not distinguish itself from next logical line. So, just make one indent here.
  5. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    line too long
  6. 
      
Christian Hammond
  1. I think after this one last set of changes, which are tiny, we'll be ready to ship it :D
  2. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This can be simplified to:
    
    for url in (url_deploy_keys, url_user_keys):
        keys_rsp = self._key_association_api_call(...)
    
        blah blah
    
        if formatted_key in ...
            return True
    
    
    Also, I'd prefer that "url" and "rsp" be suffixes, not prefixes.
  3. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
     
    *args, **kwargs. We shouldn't stray from the naming convention.
  4. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Trailing period.
  5. 
      
Karl Leuschen
David Trowbridge
  1. Ship It!
  2. 
      
Karl Leuschen
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.7.x (1d41b43). Thanks!
Loading...