Summary

Add support for hosting services.

Review Request #3073 — Created April 23, 2012 and submitted June 2, 2012, 2:03 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

david, smacleod

Description

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.

Testing Done

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.

Issues

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	May 1, 2012, 2:01 a.m.
This CharField is missing a max_length. It seems like you have max_length set for some of the fields, but not …	SM smacleod	May 1, 2012, 2:05 a.m.
> 80 characters (81). Couldn't you drop all of the strings down a line and just 4 space indent? You're …	SM smacleod	May 1, 2012, 2:06 a.m.
Too many blanks, remove a line.	SM smacleod	May 1, 2012, 2:06 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:06 a.m.
Not a show-stopper by any means, but I find it odd that we'd have two classes here that are essentially …	mike_conley	April 23, 2012, 9:12 p.m.
no max_length.	SM smacleod	May 1, 2012, 2:06 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:06 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:06 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:06 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:06 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:06 a.m.
Should this be a one-liner instead? I don't foresee Github supporting other SCM tools anytime in the near future.	mike_conley	May 1, 2012, 1:57 a.m.
Should we be using methods beginning with '_'? Aren't we saying the method is "private" when we begin with '_', …	SM smacleod	May 1, 2012, 2:08 a.m.
This looks a little funky to me. Would it be better to break the first line of the call, and …	SM smacleod	May 1, 2012, 2:10 a.m.
This mime-type can be predictably generated given the version information, should we be hard-coding it?	SM smacleod	April 24, 2012, 4:49 p.m.
The string for the mime-type is repeated here. Should we be consolidating where this string is kept for maintainability? Also, …	SM smacleod	May 1, 2012, 2:14 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:15 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:15 a.m.
> 80 characters (81).	SM smacleod	May 1, 2012, 2:15 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:15 a.m.
Why the extra ws on lines 14 and 16?	mike_conley	May 1, 2012, 1:57 a.m.
typo: accessible	mike_conley	May 1, 2012, 1:57 a.m.
typo: account	mike_conley	May 1, 2012, 1:57 a.m.
Should this be get_*? see other comment.	SM smacleod	May 1, 2012, 2:15 a.m.
Should all of these methods begin with '_'? See my comment above in the GitHub hosting service.	SM smacleod	May 1, 2012, 2:15 a.m.
Why not just define these as default values in the function header?	mike_conley	May 1, 2012, 1:58 a.m.
no max_length.	SM smacleod	May 1, 2012, 2:15 a.m.
typo: any	mike_conley	May 1, 2012, 1:58 a.m.
Should probably be l10n'able.	mike_conley	May 1, 2012, 1:58 a.m.
Should probably be l10n'able.	mike_conley	May 1, 2012, 1:59 a.m.
Should probably be l10n'able.	mike_conley	May 1, 2012, 1:59 a.m.
Should probably be l10n'able.	mike_conley	May 1, 2012, 1:59 a.m.
Should probably be l10n'able.	mike_conley	May 1, 2012, 2 a.m.
Why do you need the 'not self.errors' here? isn't it redundant from the parent if case? Or, do the 'validate_*()' …	SM smacleod	May 1, 2012, 2:17 a.m.
"thunk" ?	SM smacleod	May 1, 2012, 2:17 a.m.
Should this be 'get_*' - see my other comment.	SM smacleod	May 1, 2012, 2:18 a.m.
I first read this as 'hostings vcs'. In a product dealing with many "version control systems", my mind draws me …	SM smacleod	April 24, 2012, 4:49 p.m.
Where is this i defined? I can't seem to find it. We might want var i instead.	mike_conley	April 23, 2012, 9:15 p.m.
Or should these be indented so that the periods line up with the period before "append"?	mike_conley	April 23, 2012, 9:15 p.m.
Same as earlier - where was this i defined?	mike_conley	April 23, 2012, 9:16 p.m.
It seems like "for field in" would be nicer style.	david	May 22, 2012, 12:04 p.m.
Isn't this bad?	david	May 22, 2012, 12:05 p.m.
Alignment doesn't look quite right.	david	June 2, 2012, 1:10 a.m.

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

chipx86 April 23, 2012, 9:16 p.m.
```
Thanks for the extensive review!
```

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.

chipx86 April 23, 2012, 9:16 p.m.

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.

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.

reviewboard/hostingsvcs/models.py (Diff revision 1)
Show all issues
```
Why the extra ws on lines 14 and 16?
```
reviewboard/hostingsvcs/models.py (Diff revision 1)
Show all issues
```
typo: accessible
```
reviewboard/hostingsvcs/models.py (Diff revision 1)
Show all issues
```
typo: account
```

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

Show all issues

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

chipx86 April 23, 2012, 9:16 p.m.

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

reviewboard/scmtools/forms.py (Diff revision 1)
Show all issues
```
typo: any
```
reviewboard/scmtools/forms.py (Diff revision 1)
Show all issues
```
Should probably be l10n'able.
```
reviewboard/scmtools/forms.py (Diff revision 1)
Show all issues
```
Should probably be l10n'able.
```
reviewboard/scmtools/forms.py (Diff revision 1)
Show all issues
```
Should probably be l10n'able.
```
reviewboard/scmtools/forms.py (Diff revision 1)
Show all issues
```
Should probably be l10n'able.
```
reviewboard/scmtools/forms.py (Diff revision 1)
Show all issues
```
Should probably be l10n'able.
```

reviewboard/static/rb/js/repositoryform.js (Diff revision 1)

Is the reason for pulling this out of the loop header for performance?

chipx86 April 23, 2012, 9:16 p.m.

field could go inside the outer for loop, but this is basically for lintian checking reasons.

reviewboard/static/rb/js/repositoryform.js (Diff revision 1)

Show all issues

Where is this i defined?  I can't seem to find it.

We might want var i instead.

chipx86 April 23, 2012, 9:16 p.m.
```
A level up, in the else.
```

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"?

chipx86 April 23, 2012, 9:16 p.m.

Nope, they apply to the <option> being created.

reviewboard/static/rb/js/repositoryform.js (Diff revision 1)
Show all issues
```
Same as earlier - where was this i defined?
```
1. chipx86 April 23, 2012, 9:16 p.m.
  Shares the same i as the above.

Fix it!

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.

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

chipx86 May 1, 2012, 2:18 a.m.

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.

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

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.

reviewboard/hostingsvcs/codebasehq.py (Diff revision 1)
Show all issues
```
Too many blanks, remove a line.
```
reviewboard/hostingsvcs/fedorahosted.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
Show all issues
```
no max_length.
```

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.

chipx86 May 1, 2012, 2:18 a.m.

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 "_".

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?

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?

chipx86 May 1, 2012, 2:18 a.m.

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.

smacleod May 1, 2012, 4:08 a.m.

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/

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.

chipx86 May 1, 2012, 2:18 a.m.
```
I'll consolidate them.
```

reviewboard/hostingsvcs/gitorious.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/gitorious.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/gitorious.py (Diff revision 1)
Show all issues
```
> 80 characters (81).
```
reviewboard/hostingsvcs/googlecode.py (Diff revision 1)
Show all issues
```
no max_length.
```
reviewboard/hostingsvcs/service.py (Diff revision 1)
Show all issues
```
Should this be get_*? see other comment.
```

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.

reviewboard/hostingsvcs/sourceforge.py (Diff revision 1)
Show all issues
```
no max_length.
```

reviewboard/scmtools/forms.py (Diff revision 1)

I'd prefer "Validate that the provided tool is valid for the hosting service."

chipx86 May 1, 2012, 2:18 a.m.
```
Done.
```

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.

chipx86 May 1, 2012, 2:18 a.m.

I'll add a comment. Yes, validate_*() and _clean_*() could set errors.

reviewboard/scmtools/forms.py (Diff revision 1)
Show all issues
```
"thunk" ?
```
1. chipx86 May 1, 2012, 2:18 a.m.
  Nope, thunk. Like, call, pass through to.
reviewboard/scmtools/models.py (Diff revision 1)
Show all issues
```
Should this be 'get_*' -  see my other comment.
```

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

chipx86 May 1, 2012, 2:18 a.m.

Yeah, I wasn't totally sure what to call this, but didn't want to be too verbose.

smacleod May 1, 2012, 4:08 a.m.

I couldn't really come up with something better myself... The only thing that came to mind would be 'hostingsrvcs'.

Change Summary:

* Made the changes requested, except where there's ongoing discussion or I've explained otherwise.
* Moved the code to sit on top of RB 1.6.x, since that's where we'll be doing the first release.
* Fixed a problem when saving Custom repositories in the unit tests.

Diff:

Revision 2 (+1574 -642)

Show changes

	setup.py
	reviewboard/settings.py
	reviewboard/admin/urls.py
	reviewboard/admin/views.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/forms.py
	reviewboard/hostingsvcs/admin.py
	reviewboard/hostingsvcs/bitbucket.py
	21 more

Change Summary:

* Fixed many blockers this had on RB 1.6. I had previously developed and tested on RB 1.7, and then backported before review. This includes:
  * Added a modified version of Django's admin change_form.html with a {% block %}, allowing us to override the fieldsets for the admin page. This makes it easier to update things on our end (needed for 1.6). Django 1.4 already includes this block.
  * Removed a staticfiles import in a template.

Important additions:

* Added unit tests for field validation, API URLs, the repository form, and more.
* Added support for hosting services that don't require authorization yet (basically, everything but GitHub). Accounts are still created and stored, but they don't go through the service's authorization, and the user isn't asked for a password.
* Made it possible to determine if a hosting service account is currently authorized (basically, checking if the data locally stored about the account is what we expect -- we can invalidate API versions this way).
* Moved some code from RepositoryForm to HostingService, in order to simplify some things.
* Added hosting service info to the admin list page for repositories.

Fixes:

* Fixed issues that came up in those unit tests (such as incorrect field names used, and some problems with assumptions about hosting services having plans, and other general form mishaps).
* Fixed defaults in RepositoryForm when opening an existing repository.


At this point, I badly want to get this change in, and my testing shows it's working pretty well. The bug tracker stuff is the big P0 for the next 1.6 release, but that will be a separate change. For now, it's just stubbed out.

The interdiff will be helpful for these changes.

Description:

		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:
	~	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.
-		Unit tests.

		What I have currently is unlikely to change much (aside from comments in
		reviews), so look through it and see if things look sane.

Diff:

Revision 3 (+2152 -643)

Show changes

	setup.py
	reviewboard/settings.py
	reviewboard/admin/urls.py
	reviewboard/admin/views.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/forms.py
	reviewboard/hostingsvcs/admin.py
	reviewboard/hostingsvcs/bitbucket.py
	24 more

Very excellent. Just a few comments and questions.

reviewboard/hostingsvcs/service.py (Diff revision 3)
```
Should we log in the case where there's no entry?
```
1. chipx86 May 22, 2012, 12:06 p.m.
  Yep, probably should.

reviewboard/htdocs/media/rb/js/repositoryform.js (Diff revision 3)

Show all issues

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

chipx86 May 22, 2012, 12:06 p.m.

JavaScript doesn't do that for arrays. Or rather, it does, but the results aren't always what you expect, and should be avoided.

reviewboard/scmtools/forms.py (Diff revision 3)

Show all issues

Isn't this bad?

chipx86 May 22, 2012, 12:06 p.m.

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.

reviewboard/scmtools/forms.py (Diff revision 3)

When this says "validation" that means server-side validation, right?

chipx86 May 22, 2012, 12:06 p.m.
```
Yep.
```

reviewboard/scmtools/forms.py (Diff revision 3)
Show all issues
```
Alignment doesn't look quite right.
```
1. chipx86 May 22, 2012, 12:06 p.m.
  Looks right here. Another weird browser issue?
2. GU
  
  guest June 1, 2012, 5:24 p.m.
  also to me

reviewboard/templates/admin/change_form.html (Diff revision 3)

What is this?

chipx86 May 22, 2012, 12:06 p.m.

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.

I did a sweep of the interdiff and nothing jumped out at me. I plan to dig a little deeper soon.

Ship it!

Just one last thing I noticed on this time through.

reviewboard/htdocs/media/rb/js/repositoryform.js (Diff revision 3)
```
Should this be an === comparison?
```
1. chipx86 June 2, 2012, 1:15 a.m.
  Yep, probably should. Will fix.

Status: Closed (submitted)

Change Summary:

Pushed to release-1.6.x (b9ea340)