Summary

HostingServices can check if SSH keys are associated

Review Request #3478 — Created Nov. 7, 2012 and submitted Nov. 23, 2012, 10:15 p.m.

Information

Owner

Karl-L

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
No blank line.	chipx86	Nov. 7, 2012, 5:33 p.m.
Instead of formatting it this way, do: ('%suser/keys?access_token=%s' % (self.API_URL, ....))	chipx86	Nov. 7, 2012, 5:33 p.m.
It'd be nice to change the code to, at the best case, only check one batch of keys. So, get …	chipx86	Nov. 7, 2012, 5:35 p.m.
this can just be: return formatted_key in keys	chipx86	Nov. 7, 2012, 5:35 p.m.
Why are we factoring this out? Will more things be calling this?	chipx86	Nov. 15, 2012, 7:53 p.m.
Missing period.	chipx86	Nov. 7, 2012, 5:39 p.m.
``key``	chipx86	Nov. 7, 2012, 5:40 p.m.
``repository``	chipx86	Nov. 7, 2012, 5:39 p.m.
One dict per line.	chipx86	Nov. 7, 2012, 5:39 p.m.
Let's precompute this once outside the function.	chipx86	Nov. 7, 2012, 5:40 p.m.
Can you align the "if" with the "for" in the previous line?	david	Nov. 12, 2012, 3:31 p.m.
Why did you change the single backticks to be double backticks?	david	Nov. 15, 2012, 7:52 p.m.
Just so the entire function doesn't have to be indented, it'd be nice to just do: if not key: return …	chipx86	Nov. 15, 2012, 7:52 p.m.
I know this has gone back and forth a couple times, but really, multi-line like this should be: deploy_keys = …	chipx86	Nov. 15, 2012, 7:52 p.m.
Space after :	chipx86	Nov. 15, 2012, 7:52 p.m.
Continuation line indentation is not a multiple of four	JS jsintal	Nov. 21, 2012, 1:47 p.m.
line too long (80 > 79 characters)	JS jsintal	Nov. 21, 2012, 1:48 p.m.
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 …	chipx86	Nov. 23, 2012, 8:19 a.m.
args, *kwargs. We shouldn't stray from the naming convention.	chipx86	Nov. 23, 2012, 8:19 a.m.
Trailing period.	chipx86	Nov. 23, 2012, 8:19 a.m.
continuation line does not distinguish itself from next logical line. So, just make one indent here.	JS jsintal	Nov. 21, 2012, 1:48 p.m.
line too long	JS jsintal	Nov. 21, 2012, 1:48 p.m.

reviewboard/hostingsvcs/github.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line.
```

reviewboard/hostingsvcs/github.py (Diff revision 1)

The issue has been resolved. Show all issues

Instead of formatting it this way, do:

('%suser/keys?access_token=%s'
 % (self.API_URL, ....))

reviewboard/hostingsvcs/github.py (Diff revision 1)

The issue has been resolved. 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.

Karl-L Nov. 7, 2012, 5:46 p.m.

Agreed. Deploy keys will probably be more likely, especially if this feature gets used.

reviewboard/hostingsvcs/github.py (Diff revision 1)
The issue has been resolved. Show all issues
```
this can just be:

return formatted_key in keys
```

reviewboard/hostingsvcs/github.py (Diff revision 1)

The issue has been dropped. Show all issues

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

Karl-L Nov. 7, 2012, 5:46 p.m.

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.

reviewboard/hostingsvcs/service.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/hostingsvcs/service.py (Diff revision 1)
The issue has been resolved. Show all issues
```
``key``
```
reviewboard/hostingsvcs/service.py (Diff revision 1)
The issue has been resolved. Show all issues
```
``repository``
```
reviewboard/hostingsvcs/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
One dict per line.
```
reviewboard/hostingsvcs/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's precompute this once outside the function.
```

Change Summary:

Addressed most issues in Christian's review of r1

Diff:

Revision 2 (+102 -18)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py

reviewboard/hostingsvcs/github.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you align the "if" with the "for" in the previous line?
```

reviewboard/hostingsvcs/service.py (Diff revision 2)

The issue has been dropped. Show all issues

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

Karl-L Nov. 12, 2012, 3:30 p.m.

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.

david Nov. 14, 2012, 10:56 p.m.
```
Cool, just wondering.
```

Change Summary:

Fix some alignment issues (code style).

Diff:

Revision 3 (+102 -18)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py

reviewboard/hostingsvcs/github.py (Diff revision 3)

The issue has been resolved. 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

reviewboard/hostingsvcs/github.py (Diff revision 3)

The issue has been resolved. 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.

reviewboard/hostingsvcs/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Space after :
```

Change Summary:

Address issues in review of r3.

Diff:

Revision 4 (+108 -18)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py

Karl-L Nov. 21, 2012, 1:48 p.m.

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!

reviewboard/hostingsvcs/github.py (Diff revision 4)

The issue has been dropped. Show all issues

Continuation line indentation is not a multiple of four

Karl-L Nov. 21, 2012, 1:48 p.m.

This is shows up occasionally in the RB codebase -- this particular alignment is (slightly) more readable and trumps the PEP8 complaint in this case.

reviewboard/hostingsvcs/github.py (Diff revision 4)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)
```

reviewboard/hostingsvcs/github.py (Diff revision 4)

The issue has been dropped. Show all issues

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

reviewboard/hostingsvcs/tests.py (Diff revision 4)
The issue has been dropped. Show all issues
```
line too long
```

I think after this one last set of changes, which are tiny, we'll be ready to ship it :D

reviewboard/hostingsvcs/github.py (Diff revision 4)

The issue has been resolved. 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.

KA

Karl-L Nov. 23, 2012, 8:22 a.m.
```
Good tip -- thanks!
```

reviewboard/hostingsvcs/github.py (Diff revision 4)
The issue has been resolved. Show all issues
```
*args, **kwargs. We shouldn't stray from the naming convention.
```
reviewboard/hostingsvcs/github.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Trailing period.
```

Change Summary:

Fix style issues in r4 and test.

Diff:

Revision 5 (+101 -18)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

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