Summary

Review Board Gateway hosting service integration

Review Request #6850 — Created Jan. 31, 2015 and submitted March 12, 2015, 6:27 p.m.

Information

Owner

jyuen

Repository

Review Board

Branch

master

Bugs

Depends On

~~7006~~

Commit

6878be8...

Reviewers

Groups

reviewboard, students

People

Description

This is the Hosting Service that is meant to support rb-gateway.

Review Board Gateway is meant to be an easy to deploy hosting service that the repository owner can self host. It is supposed to provide an API in front of repositories that works closely with Review Board for retrieving files, checking file existance, etc. It also currently supports Review Board's Post Commit UI which includes the ability to get commits, get branches, and get a commit diff (change).

Testing Done

I added a git repository using the hosting service form on my local instance of review board of type 'Review Board Gateway', called rbgateway-test (with the authorized credentials). I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test. I also checked rbt post, and rbt post -u on updating an existing file, and successfully saw GET requests in rb-gateway server for requesting file blob when viewing the diff; successfully viewed the diff. Post Commit UI was also verified to work, as seen in the screenshot. When clicking on one of the commits in the Post Commit UI, a new review request was opened successfully.

Added unit tests that passed for:
- checking whether ReviewBoardGateway can find the repository
- checking whether authorization sends the expected data
- checking the service support capabilities
- checking the repository field values
- checking the get_branches function
- checking the get_commits function
- checking the get_change function

Files

Issues

Description	From	Last Updated
Is it possible to hide this checkbox? I don't believe rb-gateway's current scope includes hosting an issue tracker.	brennie	March 3, 2015, 11:36 p.m.
Col: 6 W292 no newline at end of file	reviewbot	Jan. 31, 2015, 12:52 p.m.
'is_release' imported but unused	reviewbot	Jan. 31, 2015, 12:52 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Jan. 31, 2015, 12:52 p.m.
It probably goes > 79 characters, but I think we should keep this on one line.	david	Jan. 31, 2015, 10:06 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Jan. 31, 2015, 10:19 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 1, 2015, 9:57 p.m.
'six' imported but unused	reviewbot	Feb. 9, 2015, 5:46 p.m.
'URLError' imported but unused	reviewbot	Feb. 9, 2015, 5:46 p.m.
Col: 32 E226 missing whitespace around arithmetic operator	reviewbot	Feb. 9, 2015, 5:46 p.m.
undefined name 'ugettext'	reviewbot	Feb. 9, 2015, 5:48 p.m.
undefined name 'ugettext'	reviewbot	Feb. 9, 2015, 5:48 p.m.
undefined name 'ugettext'	reviewbot	Feb. 9, 2015, 5:48 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 9, 2015, 5:46 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 9, 2015, 5:49 p.m.
This should end in a period.	david	Feb. 17, 2015, 4:33 p.m.
Can we stick this in an else clause?	david	Feb. 17, 2015, 4:33 p.m.
We should log unexpected errors.	david	Feb. 17, 2015, 4:33 p.m.
We should log unexpected errors.	david	Feb. 17, 2015, 4:33 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 12, 2015, 1:20 p.m.
undefined name 'url'	reviewbot	Feb. 18, 2015, 12:31 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 18, 2015, 12:32 p.m.
This isn't correct.	david	Feb. 21, 2015, 11:41 a.m.
This should probably be "Review Board Gateway"	david	Feb. 21, 2015, 11:41 a.m.
Checks -> Check	david	Feb. 21, 2015, 11:41 a.m.
Authorizes -> Authorize.	david	Feb. 21, 2015, 11:41 a.m.
Determines -> Determine	david	Feb. 21, 2015, 11:41 a.m.
Gets -> Get	david	Feb. 21, 2015, 11:41 a.m.
Checks -> Check	david	Feb. 21, 2015, 11:41 a.m.
This should be "Get" rather than "Gets"	david	Feb. 21, 2015, 11:41 a.m.
The summary part of the docstring should fit on one line. It should also be written using the imperative mood …	david	Feb. 21, 2015, 11:41 a.m.
Same comments here.	david	Feb. 21, 2015, 11:41 a.m.
Returns -> Return	david	Feb. 21, 2015, 11:41 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 19, 2015, 9:55 p.m.
Can you add a docstring for this class?	david	March 4, 2015, 9:59 p.m.
This should use is_new rather than _, since you're naming the gettext import _.	david	March 4, 2015, 9:59 p.m.
We should probably distinguish between 404 (which should return FileNotFoundError) and other errors (which should log and return a generic …	david	March 4, 2015, 9:59 p.m.
We should probably distinguish between 404 (which should return FileNotFoundError) and other errors (which should log and return a generic …	david	March 4, 2015, 9:59 p.m.
I'm not sure we really care to log 404s, either.	david	March 4, 2015, 9:59 p.m.
We definitely don't care about 404s here.	david	March 4, 2015, 9:59 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 22, 2015, 1:17 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 4, 2015, 10:02 p.m.
Col: 19 E128 continuation line under-indented for visual indent	reviewbot	March 5, 2015, 1:31 p.m.
Col: 19 E128 continuation line under-indented for visual indent	reviewbot	March 5, 2015, 1:31 p.m.
Col: 15 E128 continuation line under-indented for visual indent	reviewbot	March 5, 2015, 1:31 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 5, 2015, 12:40 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 5, 2015, 1:34 p.m.
Blank line between these.	brennie	March 11, 2015, 8:01 p.m.
Blank line between these.	brennie	March 11, 2015, 8:01 p.m.
Blank line between these.	brennie	March 11, 2015, 8:01 p.m.
Surround this with parenthesis and you won't need the backslashes. Also, you should be using the bytestring literal (b'') because …	brennie	March 11, 2015, 8:01 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 8, 2015, 11:12 p.m.
Blank line between these.	brennie	March 12, 2015, 5:37 p.m.
Blank line between these.	brennie	March 12, 2015, 5:37 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 11, 2015, 8:07 p.m.
Probably you should write doc strings in thrid person when you describe what the function does.	Chester	March 13, 2015, 6:48 p.m.
I would recommend if not start:	Chester	March 13, 2015, 8:03 p.m.
You could add one more parameter raw_content into the definition, when it is False, return data, headers directly, otherwise return …	Chester	March 13, 2015, 8:03 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 12, 2015, 5:39 p.m.

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py



Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py

reviewboard/hostingsvcs/rbgateway.py (Diff revision 1)
Show all issues
```
Col: 6
 W292 no newline at end of file
```
setup.py (Diff revision 1)
Show all issues
```
 'is_release' imported but unused
```

setup.py (Diff revision 1)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Style issues, forgot about review bot :)

Commit:

-	8fc6d45b364c1451debc042913c6fa770fe20ccf
+	4525497ff69b5cea31f5622d0760e2afff635675

Diff:

Revision 2 (+39 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py

setup.py (Diff revision 2)

Show all issues

It probably goes > 79 characters, but I think we should keep this on one line.

Change Summary:

Path URL fixes

Commit:

-	4525497ff69b5cea31f5622d0760e2afff635675
+	dd7e9e3f8e914cc180d9cc61c14ca5a9929d2f62

Diff:

Revision 3 (+40 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py

setup.py (Diff revision 3)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Fixed the path for the rb-gateway hosting service

Description:

~		This is a super basic hosting service set up for rb-gateway. Still a lot of work and testing to be done. I am just submitting it as a WIP for the weekly status report.
	~	This is a WIP for the Hosting Service that is meant to support rb-gateway.
	+
	+	Currently when you add a repository in the UI, of type 'Review Board Gateway' it will send a GET request to localhost:8888 (configuration to be added) to get the repository.
	+
	+	TODO:
	+	- Support more than Git
	+	- Hosting Service Subclass in need of more implementations.

Testing Done:

I added a git repository using my local instance of review board of type 'Review Board Gateway' called rbgateway-test. I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test.

Commit:

-	dd7e9e3f8e914cc180d9cc61c14ca5a9929d2f62
+	4c033c7050b1690a80578befc1bd2597c937675b

Diff:

Revision 4 (+44 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py



Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py

setup.py (Diff revision 4)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Cleaned up the form, added authentication handling

Summary:

-	[WIP] rb-gateway integration into Review Board
+	[WIP] Review Board Gateway hosting service implementation

Description:

		This is a WIP for the Hosting Service that is meant to support rb-gateway.

~		Currently when you add a repository in the UI, of type 'Review Board Gateway' it will send a GET request to localhost:8888 (configuration to be added) to get the repository.
	~	Review Board Gateway is meant to be an easy to deploy hosting service that the repository owner can self host.
-
-		TODO:
-		- Support more than Git
-		- Hosting Service Subclass in need of more implementations.

Commit:

-	4c033c7050b1690a80578befc1bd2597c937675b
+	ccab2dae926df9f6a05e210600560b5def7bac6e

Diff:

Revision 5 (+108 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py



Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py

reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
Show all issues
```
 'six' imported but unused
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
Show all issues
```
 'URLError' imported but unused
```

reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)

Show all issues

Col: 32
 E226 missing whitespace around arithmetic operator

reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
Show all issues
```
 undefined name 'ugettext'
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
Show all issues
```
 undefined name 'ugettext'
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
Show all issues
```
 undefined name 'ugettext'
```

setup.py (Diff revision 5)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Added test cases for existing functionality

Testing Done:

~		I added a git repository using my local instance of review board of type 'Review Board Gateway' called rbgateway-test. I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test.
	~	I added a git repository using the hosting service form on my local instance of review board of type 'Review Board Gateway', called rbgateway-test (with the authorized credentials). I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test.
	+
	+	Feb.9:
	+	Added unit tests that passed for:
	+	- checking whether ReviewBoardGateway can find the repository
	+	- checking whether authorization sends the expected data
	+	- checking the service support capabilities
	+	- checking the repository field values

Commit:

-	ccab2dae926df9f6a05e210600560b5def7bac6e
+	fcf9522ca7bf67b384c0c07f9ed8d82df33f5346

Diff:

Revision 6 (+173 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 6)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

get_file and get_file_exists implementations to hosting service subclass.

Testing Done:

		I added a git repository using the hosting service form on my local instance of review board of type 'Review Board Gateway', called rbgateway-test (with the authorized credentials). I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test.

		Feb.9:
		Added unit tests that passed for:
		- checking whether ReviewBoardGateway can find the repository
		- checking whether authorization sends the expected data
		- checking the service support capabilities
		- checking the repository field values
	+
	+	Feb.12:
	+	- checked rbt post, and rbt post -u on updating an existing file, successfully saw GET requests in rb-gateway server for requesting file blob when viewing the diff. Successfully viewed the diff.

Commit:

-	fcf9522ca7bf67b384c0c07f9ed8d82df33f5346
+	c835ac9bb7e6ee0391101868b6ebee8c437a7cdc

Diff:

Revision 7 (+238 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 7)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Minor updates to description / removed WIP

Summary:

-	[WIP] Review Board Gateway hosting service implementation
+	Review Board Gateway hosting service implementation

Description:

~		This is a WIP for the Hosting Service that is meant to support rb-gateway.
	~	This is a Hosting Service that is meant to support rb-gateway.

~		Review Board Gateway is meant to be an easy to deploy hosting service that the repository owner can self host.
	~	Review Board Gateway is meant to be an easy to deploy hosting service that the repository owner can self host. It is supposed to provide an API that works closely with Review Board.

Testing Done:

		I added a git repository using the hosting service form on my local instance of review board of type 'Review Board Gateway', called rbgateway-test (with the authorized credentials). I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test.

~		Feb.9:
	~	Added unit tests that passed for:
-		Added unit tests that passed for:
		- checking whether ReviewBoardGateway can find the repository
		- checking whether authorization sends the expected data
		- checking the service support capabilities
~		- checking the repository field values
	~	- checking the repository field values
-
-		Feb.12:
		- checked rbt post, and rbt post -u on updating an existing file, successfully saw GET requests in rb-gateway server for requesting file blob when viewing the diff. Successfully viewed the diff.

```
Just a few very simple comments.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 7)
Show all issues
```
This should end in a period.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 7)
Show all issues
```
Can we stick this in an else clause?
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 7)
Show all issues
```
We should log unexpected errors.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 7)
Show all issues
```
We should log unexpected errors.
```

Change Summary:

Applying David's suggestions.

Commit:

-	c835ac9bb7e6ee0391101868b6ebee8c437a7cdc
+	272528186b89cef286f44ad67a77ffc11046dd27

Diff:

Revision 8 (+246 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

reviewboard/hostingsvcs/rbgateway.py (Diff revision 8)
Show all issues
```
 undefined name 'url'
```

setup.py (Diff revision 8)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Very minor change to address Review Bot's issue.

Commit:

-	272528186b89cef286f44ad67a77ffc11046dd27
+	21857c6dadb16bc0d615d3d6a2fdd2cd17b96555

Diff:

Revision 9 (+246 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 9)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
This isn't correct.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
This should probably be "Review Board Gateway"
```

reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)

Show all issues

Checks -> Check

jyuen Feb. 20, 2015, 2:50 p.m.

I can change these but I structured a lot of the comments off of GitLab and GitHub's comments, that use Returns over Return and Checks over Check and the like. Actually now that I look, most hosting service uses the one with the 's'. While I don't know which is more accurate, I don't know if we want to keep it consistent?

david Feb. 20, 2015, 2:51 p.m.

Yeah, most of the existing code is written in passive voice, but I'll be going through and cleaning up a lot of this in the future. We should make sure any new code is PEP-257 compliant.

JY

jyuen Feb. 20, 2015, 2:52 p.m.
```
Ok will do!
```

reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
Authorizes -> Authorize.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
Determines -> Determine
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
Gets -> Get
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
Checks -> Check
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
This should be "Get" rather than "Gets"
```

reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)

Show all issues

The summary part of the docstring should fit on one line. It should also be written using the imperative mood ("Delegate" rather than the passive "Delegates")

reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
Same comments here.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
Show all issues
```
Returns -> Return
```

Change Summary:

Updating comments

Commit:

-	21857c6dadb16bc0d615d3d6a2fdd2cd17b96555
+	c3f53e35c09f3644e5809a1a7f88ea2765a8983b

Diff:

Revision 10 (+250 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 10)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Form appearance (Revision 1)

Show all issues

Is it possible to hide this checkbox? I don't believe rb-gateway's current scope includes hosting an issue tracker.

jyuen March 3, 2015, 11:36 p.m.

Hmm... on the UI, it's actually disabled (can't click checkbox). It seems other hosting services that don't have their own bug tracker also look the same (i.e. Beanstalk / Kiln on Demand). I agree it will look better hidden but I think this is something that needs to be changed on the superclass hosting service form and outside the scope of this review request.

reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)
Show all issues
```
Can you add a docstring for this class?
```

reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)

Show all issues

This should use is_new rather than _, since you're naming the gettext import _.

reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)

Show all issues

We should probably distinguish between 404 (which should return FileNotFoundError) and other errors (which should log and return a generic SCMError).

reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)

Show all issues

We should probably distinguish between 404 (which should return FileNotFoundError) and other errors (which should log and return a generic SCMError).

reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)
Show all issues
```
I'm not sure we really care to log 404s, either.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)
Show all issues
```
We definitely don't care about 404s here.
```

Change Summary:

Fixing above issues, changing URLs to follow review request #7006, verified 'Testing Done' still works.

Testing Done:

		I added a git repository using the hosting service form on my local instance of review board of type 'Review Board Gateway', called rbgateway-test (with the authorized credentials). I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test.

		Added unit tests that passed for:
		- checking whether ReviewBoardGateway can find the repository
		- checking whether authorization sends the expected data
		- checking the service support capabilities
~		- checking the repository field values
	~	- checking the repository field values
	+
	+	Additional tests:
		- checked rbt post, and rbt post -u on updating an existing file, successfully saw GET requests in rb-gateway server for requesting file blob when viewing the diff. Successfully viewed the diff.

Depends On:

+	~~7006 - Refactor routes to use URL params instead of query params~~

Commit:

-	c3f53e35c09f3644e5809a1a7f88ea2765a8983b
+	08d1ac72ad859bd472192328e66967d13d469883

Diff:

Revision 11 (+274 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 11)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Post commit functions, screenshot. Changing to WIP because I want to add unit tests for post commit next week.

Commit:

-	08d1ac72ad859bd472192328e66967d13d469883
+	4481ebd133db341762882a9873d3470bac8d5691

Diff:

Revision 12 (+348 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Added Files:

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

reviewboard/hostingsvcs/rbgateway.py (Diff revision 12)

Show all issues

Col: 19
 E128 continuation line under-indented for visual indent

reviewboard/hostingsvcs/rbgateway.py (Diff revision 12)

Show all issues

Col: 19
 E128 continuation line under-indented for visual indent

reviewboard/hostingsvcs/rbgateway.py (Diff revision 12)

Show all issues

Col: 15
 E128 continuation line under-indented for visual indent

setup.py (Diff revision 12)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Deleting additional screenshots

Summary:

-	Review Board Gateway hosting service implementation
+	[WIP] Review Board Gateway hosting service implementation

Removed Files:

Change Summary:

Minor cleanup

Commit:

-	4481ebd133db341762882a9873d3470bac8d5691
+	87afbb268eabf66368eef59f0ab6f4097798856a

Diff:

Revision 13 (+349 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 13)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Change Summary:

Added Post Commit unit testing, minor cleanups, update desc. & testing done.

Summary:

-	[WIP] Review Board Gateway hosting service implementation
+	Review Board Gateway hosting service integration

Description:

~		This is a Hosting Service that is meant to support rb-gateway.
	~	This is the Hosting Service that is meant to support rb-gateway.

~		Review Board Gateway is meant to be an easy to deploy hosting service that the repository owner can self host. It is supposed to provide an API that works closely with Review Board.
	~	Review Board Gateway is meant to be an easy to deploy hosting service that the repository owner can self host. It is supposed to provide an API in front of repositories that works closely with Review Board for retrieving files, checking file existance, etc. It also currently supports Review Board's Post Commit UI which includes the ability to get commits, get branches, and get a commit diff (change).

Testing Done:

~		I added a git repository using the hosting service form on my local instance of review board of type 'Review Board Gateway', called rbgateway-test (with the authorized credentials). I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test.
	~	I added a git repository using the hosting service form on my local instance of review board of type 'Review Board Gateway', called rbgateway-test (with the authorized credentials). I then added a .reviewboardrc to my local repository pointing to rbgateway-test. I typed 'rbt post' and it successfully posted to my local instance under rbgateway-test. I also checked rbt post, and rbt post -u on updating an existing file, and successfully saw GET requests in rb-gateway server for requesting file blob when viewing the diff; successfully viewed the diff. Post Commit UI was also verified to work, as seen in the screenshot. When clicking on one of the commits in the Post Commit UI, a new review request was opened successfully.

		Added unit tests that passed for:
		- checking whether ReviewBoardGateway can find the repository
		- checking whether authorization sends the expected data
		- checking the service support capabilities
~		- checking the repository field values
~
~		Additional tests:
~		- checked rbt post, and rbt post -u on updating an existing file, successfully saw GET requests in rb-gateway server for requesting file blob when viewing the diff. Successfully viewed the diff.
	~	- checking the repository field values
	~	- checking the get_branches function
	~	- checking the get_commits function
	~	- checking the get_change function

Commit:

-	87afbb268eabf66368eef59f0ab6f4097798856a
+	d725370303096f2d28e0dbce3b2be1579edcf651

Diff:

Revision 14 (+493 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 14)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Ship it!

```
<p>Looks good to me!</p>
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 14)
Show all issues
```
Blank line between these.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 14)
Show all issues
```
Blank line between these.
```
reviewboard/hostingsvcs/tests.py (Diff revision 14)
Show all issues
```
Blank line between these.
```

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

Show all issues

Surround this with parenthesis and you won't need the backslashes. Also, you should be using the bytestring literal (b'') because the differ will expect bytestrings and not unicode strings.

Change Summary:

Addressing style issues Barret suggested.

Commit:

-	d725370303096f2d28e0dbce3b2be1579edcf651
+	bcc00683f8e7e76548306631681085c5c498d811

Diff:

Revision 15 (+496 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 15)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Ship it!

Sorry, two little nits I missed last time around, and then I'm comfortable with landing this.

jyuen March 11, 2015, 10:40 p.m.

No problem, is this a new convention btw? (The blank between the docstring and first line of code)

brennie March 11, 2015, 10:42 p.m.

Yes, but only for classes. This is a suggestion by the pyflakes tool. It also suggests a blank line between the class line and the docstring, but that is silly so we don't use that.

reviewboard/hostingsvcs/rbgateway.py (Diff revision 15)
Show all issues
```
Blank line between these.
```
reviewboard/hostingsvcs/rbgateway.py (Diff revision 15)
Show all issues
```
Blank line between these.
```

Change Summary:

More styling fixes

Commit:

-	bcc00683f8e7e76548306631681085c5c498d811
+	6878be814aebd3f36f73d6403a0b5732dbf400ba

Diff:

Revision 16 (+498 -1)

Show changes

	setup.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/tests.py

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py



Tool: Pyflakes
Processed Files:
    setup.py
    reviewboard/hostingsvcs/rbgateway.py
    reviewboard/hostingsvcs/tests.py

setup.py (Diff revision 16)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Status: Closed (submitted)

Change Summary:

Pushed to master (2060a5d)

reviewboard/hostingsvcs/rbgateway.py (Diff revision 16)

Show all issues

Probably you should write doc strings in thrid person when you describe what the function does.

reviewboard/hostingsvcs/rbgateway.py (Diff revision 16)

Show all issues

I would recommend if not start:

jyuen March 13, 2015, 8:03 p.m.

I think you mean if start. I agree it's probably better because it handles the empty string case, but there is not much of a performance / style gain because if it is empty rb-gateway will ignore it anyway. I don't want to reopen the review request unless really necessary. But if you or anyone disagrees, feel free to comment.

Chester March 13, 2015, 8:15 p.m.

sorry I meant if start. Your implementation makes sense.

reviewboard/hostingsvcs/rbgateway.py (Diff revision 16)

Show all issues

You could add one more parameter raw_content into the definition, when it is False, return data, headers directly, otherwise return json.loads(data, headers). Because you only need raw data in get_file(), other function calls are  all required to returned json formatted string, set raw_content default to False would save you more lines of code. Therefore, you don't need to do exception ever time you call _api_get().

jyuen March 13, 2015, 8:03 p.m.

I wanted to keep _api_get self contained to just getting the raw data and header information, and raise exceptions as a result of things that have gone wrong with the API itself. This doesn't really reduce the number of exception handling because in the calling functions, such as get_commits, I would still log the error in that function regardless because telling the user get_commits did something wrong is a lot more helpful than a generic _api_get went wrong. It would save the number of times I write json.loads but I don't know if that is enough to make me want to change my intentions for _api_get. I don't think that the approach you suggested is necessary better or worse, just a different preference. :)

Chester March 13, 2015, 8:15 p.m.

You are right about the logging, it is fater to debug. But how about adding one more parameter that takes information from the caller, so the error message from _api_get() can print out where the error comes from as well. And I agree this is not a problem.