HostingServices can check if SSH keys are associated

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

Information

Review Board
master

Reviewers

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.
Description From Last Updated

No blank line.

chipx86chipx86

Instead of formatting it this way, do: ('%suser/keys?access_token=%s' % (self.API_URL, ....))

chipx86chipx86

It'd be nice to change the code to, at the best case, only check one batch of keys. So, get …

chipx86chipx86

this can just be: return formatted_key in keys

chipx86chipx86

Why are we factoring this out? Will more things be calling this?

chipx86chipx86

Missing period.

chipx86chipx86

``key``

chipx86chipx86

``repository``

chipx86chipx86

One dict per line.

chipx86chipx86

Let's precompute this once outside the function.

chipx86chipx86

Can you align the "if" with the "for" in the previous line?

daviddavid

Why did you change the single backticks to be double backticks?

daviddavid

Just so the entire function doesn't have to be indented, it'd be nice to just do: if not key: return …

chipx86chipx86

I know this has gone back and forth a couple times, but really, multi-line like this should be: deploy_keys = …

chipx86chipx86

Space after :

chipx86chipx86

Continuation line indentation is not a multiple of four

JS jsintal

line too long (80 > 79 characters)

JS jsintal

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 …

chipx86chipx86

*args, **kwargs. We shouldn't stray from the naming convention.

chipx86chipx86

Trailing period.

chipx86chipx86

continuation line does not distinguish itself from next logical line. So, just make one indent here.

JS jsintal

line too long

JS jsintal
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
    Show all issues
    No blank line.
  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
    Show all issues
    Instead of formatting it this way, do:
    
    ('%suser/keys?access_token=%s'
     % (self.API_URL, ....))
  4. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    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)
     
     
     
     
    Show all issues
    this can just be:
    
    return formatted_key in keys
  6. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
    Show all issues
    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)
     
     
    Show all issues
    Missing period.
  8. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
    Show all issues
    ``key``
  9. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
    Show all issues
    ``repository``
  10. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Show all issues
    One dict per line.
  11. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Show all issues
    Let's precompute this once outside the function.
  12. 
      
KA
david
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
     
    Show all issues
    Can you align the "if" with the "for" in the previous line?
  3. reviewboard/hostingsvcs/service.py (Diff revision 2)
     
     
    Show all issues
    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. 
      
KA
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Show all issues
    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)
     
     
     
    Show all issues
    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)
     
     
     
    Show all issues
    Space after :
  5. 
      
KA
JS
  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)
     
     
    Show all issues
    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)
     
     
    Show all issues
    line too long (80 > 79 characters)
  4. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Show all issues
    continuation line does not distinguish itself from next logical line. So, just make one indent here.
  5. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues
    line too long
  6. 
      
chipx86
  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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    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.
    1. Good tip -- thanks!
  3. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
     
    Show all issues
    *args, **kwargs. We shouldn't stray from the naming convention.
  4. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Show all issues
    Trailing period.
  5. 
      
KA
david
  1. Ship It!
  2. 
      
KA
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.7.x (1d41b43). Thanks!