Administrators can associate server SSH key from the Repository form

Review Request #3488 — Created Nov. 10, 2012 and submitted

Information

Review Board
master

Reviewers

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_conleymike_conley

extra_cleaned_data should not be considered clean? Sounds like it needs to be renamed then.

mike_conleymike_conley

Why did this move?

chipx86chipx86

What's needed for this?

chipx86chipx86

You can combine this using: not getattr(self.extra_form_data, 'associate_ssh_key', None)

chipx86chipx86

Should be one statement.

chipx86chipx86

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?

daviddavid

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.

daviddavid

Alignment looks off.

chipx86chipx86

No need for parens.

chipx86chipx86

Use local_site_reverse. We need it in case the admin UI is on a LocalSite and not the main admin UI. …

chipx86chipx86

This would be nicer and less blocky if you start it indented 4 spaces on the next line.

chipx86chipx86

For multi-line comments: /* * blah * blah */

chipx86chipx86

Been trying to think on this label. You know, let's make it more of a sentence. Let's just set this …

chipx86chipx86

"repository deploy keys" is GitHub-specific. Just say, "list of authorized SSH keys on the hosting service."

chipx86chipx86
KA
  1. 
      
    1. Ignore -- attempted to post review while submitting a new request (instead of making the request public first).
  2. 
      
KA
  1. 
      
    1. I forgot that  you can select multiple lines for comments. Probably best to look at the following comments from the diff view for context.
  2. reviewboard/hostingsvcs/forms.py (Diff revision 1)
     
     
    A bit of a hack; this is purely for aesthetic reasons as having the associate ssh key field appear anywhere else looks strange.
  3. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Moved this chunk intentionally so that _load_hosting_service() has access to the public key.
  4. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Coming as part of r/3478/
  5. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    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.
  6. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    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.
  7. 
      
KA
mike_conley
  1. Hey Karl - this is looking really good. I've got nothing major to complain about, just a few nits.
  2. reviewboard/hostingsvcs/forms.py (Diff revision 1)
     
     
     
     
    Show all issues
    Nit - since these are still within the second level of parentheses, they should be indented one more space.
  3. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues
    extra_cleaned_data should not be considered clean? Sounds like it needs to be renamed then.
  4. 
      
KA
KA
chipx86
  1. 
      
  2. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    Why did this move?
    1. The presence of a key was for the hosting service forms to disable the "Associate SSH key" field. They were getting loaded before the key was being set. This move will probably be reversed when the field is added to the RepositoryForm instead.
  3. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
     
    Show all issues
    What's needed for this?
    1. The patch in r/3478 has the code to do the check. It will get added if/when the patch is applied.
  4. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
     
    Show all issues
    You can combine this using:
    
    not getattr(self.extra_form_data, 'associate_ssh_key', None)
  5. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    Should be one statement.
  6. 
      
KA
KA
KA
  1. 
      
  2. reviewboard/scmtools/forms.py (Diff revision 4)
     
     
    Show all issues
    The name of the fieldset is temporary. Perhaps it could be "Repository Auto-configuration" or "Repository Deploy Keys"? Other suggestions?
  3. reviewboard/scmtools/forms.py (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues
    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).
  4. 
      
KA
KA
david
  1. 
      
  2. reviewboard/scmtools/forms.py (Diff revision 4)
     
     
    Show all issues
    Can we call this NO_KEY_HELP_FMT, since it needs to have the URL formatted into it?
  3. reviewboard/static/rb/js/repositoryform.js (Diff revision 4)
     
     
     
    Show all issues
    In this case, I think a line that's a little too long is more readable than this wrapping.
  4. 
      
KA
chipx86
  1. 
      
  2. reviewboard/scmtools/forms.py (Diff revision 5)
     
     
     
     
    Show all issues
    Alignment looks off.
  3. reviewboard/scmtools/forms.py (Diff revision 5)
     
     
     
     
    Show all issues
    No need for parens.
  4. reviewboard/scmtools/forms.py (Diff revision 5)
     
     
    Show all issues
    Use local_site_reverse. We need it in case the admin UI is on a LocalSite and not the main admin UI.
    
    You'll need to pass in the local_site_name as well.
  5. reviewboard/scmtools/forms.py (Diff revision 5)
     
     
     
     
     
     
     
     
    Show all issues
    This would be nicer and less blocky if you start it indented 4 spaces on the next line.
  6. reviewboard/static/rb/js/repositoryform.js (Diff revision 5)
     
     
     
    Show all issues
    For multi-line comments:
    
    /*
     * blah
     * blah
     */
  7. 
      
KA
chipx86
  1. 
      
  2. reviewboard/scmtools/forms.py (Diff revision 6)
     
     
    Show all issues
    Been trying to think on this label. You know, let's make it more of a sentence. Let's just set this to: "Associate my SSH key with the hosting service"
  3. reviewboard/scmtools/forms.py (Diff revision 6)
     
     
     
    Show all issues
    "repository deploy keys" is GitHub-specific. Just say, "list of authorized SSH keys on the hosting service."
  4. 
      
KA
david
  1. Ship It!
  2. 
      
KA
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.7.x (0f0c5c3). Thanks!