Add support for hosting services.

Review Request #3073 — Created April 23, 2012 and submitted

Information

Review Board

Reviewers

Add support for hosting services.

This introduces top-level support for hosting services. Previously,
Review Board had a basic concept of hosting services when configuring a
repository. They were nothing more than some pattern matches that
provided helpful fields for configuring a repository's information.

Now, we have a top-level concept of a hosting service. Subclasses of
HostingService are used to provide specific configuration forms for
repositories, provide an ability to request an authorization token from
the hosting service, and provide basic file access.

The various fields (such as a GitHub repository name or whether it's a
public personal account or a private organization) are now stored along
with the Repository in its new extra_data field. This is used later for
both repository access and configuration.

When setting up a repository for a hosting service, the user is expected
to link an account or use an existing linked account for that service.
When linking an account on hosting services that require authorization,
Review Board will perform the authorization request and, if successful,
store any service-specific tokens or data in the HostingServiceAccount
model.

All the existing hosting services have been moved into HostingService
subclasses. Existing configurations will no longer show a hosting
service tied to the account. Instead, they will just appear as custom
repositories. Users are always free to reconfigure their accounts.

GitHub is the first hosting service we support in an extended way. We
get an authorization token, store it, and use it for file requests with
their v3 API.

TODO for future changes:

* Fix bug trackers. Right now, the code is temporarily disabled. I will
  be moving to custom forms for configuring this, just like we have with
  repository configuration now.

* Auto-generate an SSH key when needed, and upload to the account.

What I have currently is unlikely to change much (aside from comments in
reviews), so look through it and see if things look sane.
Lots of manual testing with GitHub accounts and with different
configurations of options. Triggered most of the validations that could
naturally be triggered in the web UI.
Description From Last Updated

Is there a reason to make the new methods 'get_file_exists' instead of just 'file_exists'? I think we'd benefit having consistent …

SM smacleod

This CharField is missing a max_length. It seems like you have max_length set for some of the fields, but not …

SM smacleod

> 80 characters (81). Couldn't you drop all of the strings down a line and just 4 space indent? You're …

SM smacleod

Too many blanks, remove a line.

SM smacleod

no max_length.

SM smacleod

Not a show-stopper by any means, but I find it odd that we'd have two classes here that are essentially …

mike_conleymike_conley

no max_length.

SM smacleod

no max_length.

SM smacleod

no max_length.

SM smacleod

no max_length.

SM smacleod

no max_length.

SM smacleod

no max_length.

SM smacleod

Should this be a one-liner instead? I don't foresee Github supporting other SCM tools anytime in the near future.

mike_conleymike_conley

Should we be using methods beginning with '_'? Aren't we saying the method is "private" when we begin with '_', …

SM smacleod

This looks a little funky to me. Would it be better to break the first line of the call, and …

SM smacleod

This mime-type can be predictably generated given the version information, should we be hard-coding it?

SM smacleod

The string for the mime-type is repeated here. Should we be consolidating where this string is kept for maintainability? Also, …

SM smacleod

no max_length.

SM smacleod

no max_length.

SM smacleod

> 80 characters (81).

SM smacleod

no max_length.

SM smacleod

Why the extra ws on lines 14 and 16?

mike_conleymike_conley

typo: accessible

mike_conleymike_conley

typo: account

mike_conleymike_conley

Should this be get_*? see other comment.

SM smacleod

Should all of these methods begin with '_'? See my comment above in the GitHub hosting service.

SM smacleod

Why not just define these as default values in the function header?

mike_conleymike_conley

no max_length.

SM smacleod

typo: any

mike_conleymike_conley

Should probably be l10n'able.

mike_conleymike_conley

Should probably be l10n'able.

mike_conleymike_conley

Should probably be l10n'able.

mike_conleymike_conley

Should probably be l10n'able.

mike_conleymike_conley

Should probably be l10n'able.

mike_conleymike_conley

Why do you need the 'not self.errors' here? isn't it redundant from the parent if case? Or, do the 'validate_*()' …

SM smacleod

"thunk" ?

SM smacleod

Should this be 'get_*' - see my other comment.

SM smacleod

I first read this as 'hostings vcs'. In a product dealing with many "version control systems", my mind draws me …

SM smacleod

Where is this i defined? I can't seem to find it. We might want var i instead.

mike_conleymike_conley

Or should these be indented so that the periods line up with the period before "append"?

mike_conleymike_conley

Same as earlier - where was this i defined?

mike_conleymike_conley

It seems like "for field in" would be nicer style.

daviddavid

Isn't this bad?

daviddavid

Alignment doesn't look quite right.

daviddavid
mike_conley
  1. Christian:
    
    For such an extensive change, I'm surprised at how readable it is.  Kudos.
    
    Just a bunch of nits I found from inspection.  I haven't taken the patch out for a spin yet.
    
    Thanks,
    
    -Mike
    1. Thanks for the extensive review!
  2. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Not a show-stopper by any means, but I find it odd that we'd have two classes here that are essentially equivalent.  Same with the public and private organization forms.
    
    If there's a good reason for this, feel free to drop.  Else, we might just want to consolidate to a generic GitHubForm and GitHubOrgForm.
    1. Yeah, the reason is that each form gets injected into the HTML, and we can't have duplicate field names or everything will go wonky. So, each form has a namespaced field name. Sort of a necessity. I played around with ideas for it, but it just made things wayyy more complicated.
  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
    Show all issues
    Should this be a one-liner instead?  I don't foresee Github supporting other SCM tools anytime in the near future.
  4. reviewboard/hostingsvcs/models.py (Diff revision 1)
     
     
     
     
    Show all issues
    Why the extra ws on lines 14 and 16?
  5. reviewboard/hostingsvcs/models.py (Diff revision 1)
     
     
    Show all issues
    typo: accessible
  6. reviewboard/hostingsvcs/models.py (Diff revision 1)
     
     
    Show all issues
    typo: account
  7. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
     
    Show all issues
    Why not just define these as default values in the function header?
    1. I don't know. Copied this from RBTools :) Maybe at one point we put values in them. (If you have a default value of {} and you put stuff into it, it works like a static variable.)
  8. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues
    typo: any
  9. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues
    Should probably be l10n'able.
  10. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
     
    Show all issues
    Should probably be l10n'able.
  11. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues
    Should probably be l10n'able.
  12. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues
    Should probably be l10n'able.
  13. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
     
     
    Show all issues
    Should probably be l10n'able.
  14. reviewboard/static/rb/js/repositoryform.js (Diff revision 1)
     
     
     
    Is the reason for pulling this out of the loop header for performance?
    1. field could go inside the outer for loop, but this is basically for lintian checking reasons.
  15. Show all issues
    Where is this i defined?  I can't seem to find it.
    
    We might want var i instead.
    1. A level up, in the else.
  16. reviewboard/static/rb/js/repositoryform.js (Diff revision 1)
     
     
     
    Show all issues
    Or should these be indented so that the periods line up with the period before "append"?
    1. Nope, they apply to the <option> being created.
  17. Show all issues
    Same as earlier - where was this i defined?
    1. Shares the same i as the above.
  18. 
      
SM
  1. Wow, quite the change. Overall it's looking really good, I couldn't find any major issues.
    
    I had problems getting the patch working. ./reviewboard/manage.py evolve is crashing on me, but It looks like something completely unrelated to this change. I'll investigate more and try and test the change once I get everything working.
    
    I skipped over the javascript in this review, since I'm far from familiar with it. I might come back and take a good look through it when I get the change working.
    
    Note: The hostingsvcs/__init__.py is missing from the patch.
  2. reviewboard/diffviewer/forms.py (Diff revision 1)
     
     
    Show all issues
    Is there a reason to make the new methods 'get_file_exists' instead of just 'file_exists'?
    
    I think we'd benefit having consistent methods when dealing with the repository or the scm tool directly. (Looking at the new hosting_services model, it appears to use the 'get_*' signature as well, but the tools are still using 'file_exists')
    1. Yeah, I don't like that I named it "file_exists," but I'm not eager to go breaking that API right now. Generally, info-retrieval functions should be get_* or is_* or has_* or whatever.
  3. reviewboard/hostingsvcs/codebasehq.py (Diff revision 1)
     
     
    Show all issues
    This CharField is missing a max_length.
    
    It seems like you have max_length set for some of the fields, but not others. I couldn't discern a scheme for whether or not the field was given a max_length. This applies to many of the services, not just codebasehq.
    
    If I'm not catching something here about why the max_lengths are missing, please drop all the issues (and sorry for the spam).
  4. reviewboard/hostingsvcs/codebasehq.py (Diff revision 1)
     
     
    Show all issues
    > 80 characters (81).
    
    Couldn't you drop all of the strings down a line and just 4 space indent? You're probably ignoring this on purpose for style and consistency, so feel free to drop this issue.
  5. reviewboard/hostingsvcs/codebasehq.py (Diff revision 1)
     
     
     
     
    Show all issues
    Too many blanks, remove a line.
  6. reviewboard/hostingsvcs/fedorahosted.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  7. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  8. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  9. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  10. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  11. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  12. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  13. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    Should we be using methods beginning with '_'? Aren't we saying the method is "private" when we begin with '_', and not promising it will be stable? By having this hosting service rely on these methods you're setting a precedent for future hosting services.
    1. Sort of intentional right now. The work going into it today is using these methods, but I don't really want to keep them as-is long-term. What I really want is a better set of HTTP/API access libs that we can use. Maybe something that RBTools can use as well. Until then, usage of these APIs are kind of in a "I know what I'm getting myself into" mode. Hence the "_".
  14. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    This looks a little funky to me. Would it be better to break the first line of the call, and just 4-space indent all of the arguments?
  15. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    This mime-type can be predictably generated given the version information, should we be hard-coding it?
    1. Predictably generated how? You mean from the server somehow?
      
      We want to be explicit in the version of this API that we're using. I don't want to get into a situation where we end up with an unexpectedly new mimetype based on something generated.
      
      I may just be misunderstanding.
    2. Sorry, I wasn't very clear. All of GitHub's API mimetypes take the form:
      
          application/vnd.github[.version].param[+json]
      
      So we could generate the mimetype using the version information we have. Whether this is necessary, or even provides anything extra is up to you.
      Taken from: http://developer.github.com/v3/mime/
  16. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    The string for the mime-type is repeated here. Should we be consolidating where this string is kept for maintainability? Also, see above about generating it.
    1. I'll consolidate them.
  17. reviewboard/hostingsvcs/gitorious.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  18. reviewboard/hostingsvcs/gitorious.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  19. reviewboard/hostingsvcs/gitorious.py (Diff revision 1)
     
     
    Show all issues
    > 80 characters (81).
  20. reviewboard/hostingsvcs/googlecode.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  21. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
    Show all issues
    Should this be get_*? see other comment.
  22. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
     
     
    Show all issues
    Should all of these methods begin with '_'? See my comment above in the GitHub hosting service.
  23. reviewboard/hostingsvcs/sourceforge.py (Diff revision 1)
     
     
    Show all issues
    no max_length.
  24. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    I'd prefer "Validate that the provided tool is valid for the hosting service."
  25. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues
    Why do you need the 'not self.errors' here? isn't it redundant from the parent if case?
    
    Or, do the 'validate_*()' calls set self.errors? If this is the case, a comment might be nice here to explain the redundancy. 
    1. I'll add a comment. Yes, validate_*() and _clean_*() could set errors.
  26. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues
    "thunk" ?
    1. Nope, thunk. Like, call, pass through to.
  27. reviewboard/scmtools/models.py (Diff revision 1)
     
     
    Show all issues
    Should this be 'get_*' -  see my other comment.
  28. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
    I first read this as 'hostings vcs'. In a product dealing with many "version control systems", my mind draws me to that instead of the desired 'hosting services'.
    1. Yeah, I wasn't totally sure what to call this, but didn't want to be too verbose.
    2. I couldn't really come up with something better myself... The only thing that came to mind would be 'hostingsrvcs'.
  29. 
      
chipx86
chipx86
david
  1. Very excellent. Just a few comments and questions.
  2. reviewboard/hostingsvcs/service.py (Diff revision 3)
     
     
    Should we log in the case where there's no entry?
    1. Yep, probably should.
  3. Show all issues
    It seems like "for field in" would be nicer style.
    1. JavaScript doesn't do that for arrays. Or rather, it does, but the results aren't always what you expect, and should be avoided.
  4. reviewboard/scmtools/forms.py (Diff revision 3)
     
     
     
    Show all issues
    Isn't this bad?
    1. This is intentional. I'm temporarily disabling the bug tracker field stuff. The next change will implement that. It's another large block of work (smaller than this though), so I didn't want to group it in with everything else.
      
      We won't ship with this.
  5. reviewboard/scmtools/forms.py (Diff revision 3)
     
     
     
    When this says "validation" that means server-side validation, right?
  6. reviewboard/scmtools/forms.py (Diff revision 3)
     
     
     
    Show all issues
    Alignment doesn't look quite right.
    1. Looks right here. Another weird browser issue?
    2. also to me
  7. What is this?
    1. We'll eventually want this on every page, but this is from Django's admin page. Basically, this special URL provides the whole translation dictionary for the given language for use in JavaScript.
  8. 
      
SM
  1. I did a sweep of the interdiff and nothing jumped out at me. I plan to dig a little deeper soon.
  2. 
      
david
  1. Just one last thing I noticed on this time through.
  2. Should this be an === comparison?
    1. Yep, probably should. Will fix.
  3. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.6.x (b9ea340)