Administrators can associate server SSH key from the Repository form
Review Request #3488 — Created Nov. 10, 2012 and submitted
Administrator(s) of a Review Board instance are given the option to automatically associate the instance's SSH key with new/existing repositories, if supported by the hosting service, from the Repository form.
Manually ensured key association completed under the conditions required
for existing repositories:
- created new repository using hosting service that supports SSH key
association (GitHub)
- after repository created, edited repository, checking associate
SSH key form field.
- confirmed key uploaded to GitHub
Manually ensured key association completed under the following conditions
for new repositories:
- from the admin, added new repository, using GitHub as the hosting
service, and setting the "Associate SSH key" field.
- confirmed key uploaded to GitHub
Manually confirmed that correct error message returned when key
association failed. Confirmed that key association not attempted when
the form already contained errors.
| Description | From | Last Updated |
|---|---|---|
|
Nit - since these are still within the second level of parentheses, they should be indented one more space. |
 mike_conley
|
|
|
extra_cleaned_data should not be considered clean? Sounds like it needs to be renamed then. |
mike_conley
|
|
|
Why did this move? |
 chipx86
|
|
|
What's needed for this? |
chipx86
|
|
|
You can combine this using: not getattr(self.extra_form_data, 'associate_ssh_key', None) |
chipx86
|
|
|
Should be one statement. |
chipx86
|
|
|
The name of the fieldset is temporary. Perhaps it could be "Repository Auto-configuration" or "Repository Deploy Keys"? Other suggestions? |
KA Karl-L | |
|
Can we call this NO_KEY_HELP_FMT, since it needs to have the URL formatted into it? |
 david
|
|
|
It might also be worth adding a check to see if the key is already associated here (for existing repositories). … |
KA Karl-L | |
|
In this case, I think a line that's a little too long is more readable than this wrapping. |
david
|
|
|
Alignment looks off. |
chipx86
|
|
|
No need for parens. |
chipx86
|
|
|
Use local_site_reverse. We need it in case the admin UI is on a LocalSite and not the main admin UI. … |
chipx86
|
|
|
This would be nicer and less blocky if you start it indented 4 spaces on the next line. |
chipx86
|
|
|
For multi-line comments: /* * blah * blah */ |
chipx86
|
|
|
Been trying to think on this label. You know, let's make it more of a sentence. Let's just set this … |
chipx86
|
|
|
"repository deploy keys" is GitHub-specific. Just say, "list of authorized SSH keys on the hosting service." |
chipx86
|
KA
-
-
A bit of a hack; this is purely for aesthetic reasons as having the associate ssh key field appear anywhere else looks strange.
-
-
-
This may not be the right approach, but the call to the super's full_clean(), in turn, performs all our custom validation. _clean_ssh_key_association() needs some of the extra_cleaned_data to work. extra_cleaned_data eventually gets tacked on to cleaned_data, but at this point, it should not be considered clean.
-
So far, this patch won't associate ssh keys with newly created repositories. extra_data isn't tacked on to the Repository until save is called, at which point we can't back out if there's a ValidationError. We need that extra_data, though to make API calls. The potential solutions I can think of are: a) pass associate_ssh_key a copy of the potential repository, with the extra data tacked on (it can be assumed, at this point, that the data is valid) or b) associate the key in save(), saving the repo and posting up a warning that the key association didn't work. Option b) has already been discussed, and isn't preferred.
KA
- Change Summary:
-
Added screenshots of the form field, for reference.
- Screenshots:
-
Form field, when SSH key existsForm field, when SSH key does not exist.
KA
- Change Summary:
-
This revision allows new repositories to have their keys associated on creation, and adds a more user friendly error message for failed attempts to associate the key. The same tests were conducted on this revision and they passed excepting one situation, likely exclusive to hosting services with more than one repository type. If a repository is created or edited, and "associate ssh key" is checked, and the form does not validate, then subsequent attempts to save the repository with "associate ssh key" unchecked will fail. The problem appears to be that the field is duplicated for each repository type, and so, even if the visible checkbox is unchecked, the hidden checkboxes are resubmitted as checked. Also in this update: - Fix an indentation issue - Rename extra_cleaned_data to extra_form_data, since the data isn't *quite* validated
KA
- Testing Done:
-
Manually ensured key association completed under the conditions required for existing repositories:
- created new repository using hosting service that supports SSH key association (GitHub) - after repository created, edited repository, checking associate SSH key. - confirmed key uploaded to GitHub ~ Note: does NOT work for new repositories, hence one of the reasons for the patch being in WIP status.
~ Manually ensured key association completed under the following conditions for new repositories:
+ - from the admin, added new repository, using GitHub as the hosting service, and setting the + the "Associate SSH key" field. + - confirmed key uploaded to GitHub + + Manually confirmed that correct error message returned when key association failed. Confirmed
+ that key association not attempted when the form already contained errors.
KA
- Change Summary:
-
Part of this patch's approach to adding a key association control to the form is going to change in subsequent revisions. So, stand by. Fixed issues identified in revision 2: * combined (using getattr) a conditional statement that checked for the existence of a key in a dict, then the value of the key * reformatted an error message
KA
- Change Summary:
-
* Moved the key association field to the main RepositoryForm * added field as part of its own fieldset * Added code to check to see if key is associated before attempting to upload the key * Modified the repository form JS to remove the key association fieldset when working with a hosting service that does not support key association
KA
-
-
The name of the fieldset is temporary. Perhaps it could be "Repository Auto-configuration" or "Repository Deploy Keys"? Other suggestions?
-
It might also be worth adding a check to see if the key is already associated here (for existing repositories). At best, the user could see that the current key is already associated (by the field being checked or with other feedback, like a badge or the like). The cost is another API call and some additional complexity tying the state of the field to specific hosting service(s).
KA
- Change Summary:
-
Move review request out of WIP status.
- Summary:
-
[WIP] Admins can associate server SSH key from the Repository formAdministrators can associate server SSH key from the Repository form
- Testing Done:
-
~ Manually ensured key association completed under the conditions required for existing repositories:
~ - created new repository using hosting service that supports SSH key association (GitHub) ~ - after repository created, edited repository, checking associate SSH key. ~ Manually ensured key association completed under the conditions required
~ for existing repositories: ~ - created new repository using hosting service that supports SSH key + association (GitHub) + - after repository created, edited repository, checking associate + SSH key form field. - confirmed key uploaded to GitHub ~ Manually ensured key association completed under the following conditions for new repositories:
~ - from the admin, added new repository, using GitHub as the hosting service, and setting the ~ the "Associate SSH key" field. ~ Manually ensured key association completed under the following conditions
~ for new repositories: ~ - from the admin, added new repository, using GitHub as the hosting + service, and setting the "Associate SSH key" field. - confirmed key uploaded to GitHub ~ Manually confirmed that correct error message returned when key association failed. Confirmed
~ that key association not attempted when the form already contained errors. ~ Manually confirmed that correct error message returned when key
~ association failed. Confirmed that key association not attempted when + the form already contained errors. - Screenshots:
-
Form field, when SSH key existsForm field, when SSH key does not exist.
KA
- Change Summary:
-
Fix a couple of issues (variable name, JS line wrapping) pointed out by David in a review of r4.