Summary

Allow automatic SSH key association when adding/editing GitHub repositories

Review Request #3423 — Created Oct. 16, 2012 and submitted Nov. 3, 2012, 11:51 p.m.

Information

Owner

Karl-L

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

Unit test for GitHub auto key association passes. Tested that key is delivered
and added to the deploy keys of a GitHub project.

Issues

Description	From	Last Updated
Add a trailing comma.	chipx86	Oct. 16, 2012, 5:42 p.m.
Remove this blank line.	chipx86	Oct. 16, 2012, 5:42 p.m.
We should log what we get in this case. Both the HTTP exception data and the exception when loading/reading.	chipx86	Oct. 16, 2012, 5:44 p.m.
If we're going to log something, we need to have some more context, so it's clear when reading the log …	chipx86	Oct. 23, 2012, 6:06 p.m.
Must pass the local_site tied to the repository.	chipx86	Oct. 16, 2012, 5:42 p.m.
We don't use the "if else " form.	chipx86	Oct. 16, 2012, 5:42 p.m.
Make sure the setUp and tearDown functions in this class back up and restore these functions.	chipx86	Oct. 16, 2012, 5:43 p.m.
Add a trailing comma.	chipx86	Oct. 16, 2012, 5:43 p.m.
Two blank lines.	chipx86	Nov. 1, 2012, 6:22 p.m.
Align SSHKeyAssociationError with AuthorizationError.	chipx86	Nov. 1, 2012, 6:22 p.m.
You need to return something if there is no key. Actually, is this even called without a key? If not, …	chipx86	Nov. 1, 2012, 6:22 p.m.
We should just always raise this. Nothing should ever call this without checking, so it's a bug either way.	chipx86	Nov. 1, 2012, 6:22 p.m.

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.

chipx86 Oct. 16, 2012, 1:10 p.m.

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.

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?

chipx86 Oct. 16, 2012, 1:10 p.m.

Let's keep it here until we know we need this exact data elsewhere.

reviewboard/hostingsvcs/github.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a trailing comma.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Remove this blank line.
```

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

The issue has been resolved. Show all issues

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

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

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

Karl-L Oct. 16, 2012, 5:44 p.m.

Went with raising an exception only (no logging) -- is it worth it to do both here?

reviewboard/hostingsvcs/github.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Must pass the local_site tied to the repository.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We don't use the "if <X> else <Y>" form.
```

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

The issue has been resolved. Show all issues

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

reviewboard/hostingsvcs/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a trailing comma.
```

Change Summary:

Address the issues pointed out in revision 1.

Diff:

Revision 2 (+99 -1)

Show changes

	reviewboard/hostingsvcs/errors.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/tests.py

Heads up: The new SSH code has been committed to release-1.6.x and master.

Change Summary:

Added the associate_ssh_key to the base HostingService class, along with class configuration flag. For child hosting services that support ssh key association, the flag (supports_ssh_key_association) should be set, and the associate_ssh_key method should be implemented.

Branch:

+	master

Diff:

Revision 3 (+113 -1)

Show changes

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

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.)

reviewboard/hostingsvcs/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Two blank lines.
```
reviewboard/hostingsvcs/github.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Align SSHKeyAssociationError with AuthorizationError.
```

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

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

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

The issue has been resolved. Show all issues

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

Change Summary:

Address issues with last revision.

Diff:

Revision 4 (+112 -1)

Show changes

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

Change Summary:

Move review request out of WIP status.

Summary:

-	WIP: Allow automatic SSH key association when adding/editing repositories
+	Allow automatic SSH key association when adding/editing GitHub repositories

Description:

~		Work-in-progress review for SSH key association.
~
~		The goal of this feature is to allow administrators to (optionally) associate the RB server's SSH key with a repository being hosted . The reason for the feature is to simplify repository set up, removing the additional legwork the admin would need to do to get RB talking with a hosted repository.
~
~		Note: The new SSHClient (being developed separately) impacts this code, but not enough to change its basic layout.
	~	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.

Testing Done:

~		Unit test for GitHub auto key association passes. Tested that key is delivered and added to the deploy keys of a GitHub project.
	~	Unit test for GitHub auto key association passes. Tested that key is delivered
	+	and added to the deploy keys of a GitHub project.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (afc96e2)