rb-gateway
Review Request #6831 — Created Jan. 25, 2015 and submitted
This is the beginnings of the rb-gateway project. It is meant to extend existing ReviewBoard functionalities to provide better support for repositories.
Currently, this is a wireframe set up using Google Go, and the git2go library for better Git support.
Structure is as followed:
- repository.go : Base repository structure and interface
- git_repository.go : The Git implementation of repository.go
- routes.go : Handles all web services and routing
- util.go : Handles configuration loading and repository lookup
- sample_config.json : Sample configuration file using .json format
- main.go : main() lives here, access logging
- auth.go : Basic Auth implementation
- session.go : Represents the current user sessionLocal use case:
1. Specify repo_name = path in config.json (see sample config)
2. Specify credentials in config.json
3. Specify the port you want the server to run on
2. After the server is started, go to ./localhost:<port>?repo=repo_name&id=sha
3. A file should download containing the requested blob.Link the old review request before pointing to rb-gateway: https://reviews.reviewboard.org/r/6825/
The server uses Basic Auth. Currently authenticating against a single user in the config file, this may need to change. Basic Auth expects the username:password combination in the /session path, and then encrypts that information and stores it in a session. The encrypted information is stored as a PRIVATE-TOKEN (similiar to the Gitlab API), and is expected in future http requests.
Routes are available for getting file blob and checking file existance based on either the Git revision, or the Git commit id, file path pair. It currently does not support other SCM aside from Git.
Authentication:
- curl -u username:password http://localhost:8002/session returns 200 OK and the PRIVATE-TOKEN
- Set up a repository succesfully in RB using a valid user / pass combo, and saw the access log in rb-gatewayFile and File Exists
- Tested the following paths via browser, review board, and curl for HEAD and GET methods; returned expected values for valid requests:
- http://localhost:8002/file?id=<revision-sha> (returned 200 OK, and file blob if GET)
- http://localhost:8002/file?commit=<commit>&path=<path> (returned 200 OK, and file blob if GET)
Description | From | Last Updated |
---|---|---|
Can you do this? cmd := exec.Command("git", "show", id) cmd.Dir = repo.Path ... Same above. |
david | |
Can you add a default case to log an error? |
david | |
How about storing the "Repos" var as a map from name to repo? |
david | |
These should actually be the same URL (and the same handler). Inside the handler, we can switch between "get file" … |
david | |
Just a nit pick, but underlines with = and - in Markdown should be the same length as the text … |
brennie | |
Can we call this FileExists? (remove the "Get") |
david | |
It might be nice to log the URLs that are accessed and the resulting response status/payload size (kind of an … |
david | |
These two lines could be combined (if repo.GetFileExists(id) {) |
david | |
As I read through this, I'm wondering if instead of logging, we should make this function return the error (use … |
david | |
This is pretty verbose. Can we just have a single line in the log? (get rid of the =====s) |
david | |
To be honest, this comment doesn't really add anything useful (the method name is quite self-explanatory). |
david | |
Does this need to be exported? It looks like we don't use it outside of this file. That said, it … |
david | |
These cases can probably be combined (we expect either an err or a blob) |
david | |
Instead of grabbing github master, how about using "gopkg.in/libgit2/git2go.v22"? That way people building this can just do go get to … |
david | |
These two comments don't really add very much (it's pretty easy to find these functions in a relatively small codebase, … |
david | |
This doesn't need to be exported out of this file. |
david | |
This doesn't need to be exported out of this file. |
david | |
This doesn't need to be exported out of this file. |
david | |
This doesn't seem to provide any advantage over just calling json.Marshal(session) directly. |
david | |
Can you make sure that there's always a space after a colon? |
david | |
You have an "scm" field in the repositories in the config. That seems like a more reliable way of detecting … |
david |
Change Summary:
Ran go fmt for better formatting
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+194) |
-
Tool: PEP8 Style Checker Ignored Files: git_repository.go repository.go main.go web_service.go util.go config.json Tool: Pyflakes Ignored Files: git_repository.go repository.go main.go web_service.go util.go config.json
-
-
git_repository.go (Diff revision 2) Can you do this?
cmd := exec.Command("git", "show", id) cmd.Dir = repo.Path ...
Same above.
-
-
-
web_service.go (Diff revision 2) These should actually be the same URL (and the same handler). Inside the handler, we can switch between "get file" and "get file exists" by checking whether
r.Method
is"GET"
or"HEAD"
, respectively.
Change Summary:
Fixing issues and general improvements + git2go integration and README
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+261) |
-
Tool: PEP8 Style Checker Ignored Files: git_repository.go repository.go main.go web_service.go util.go config.json README.md Tool: Pyflakes Ignored Files: git_repository.go repository.go main.go web_service.go util.go config.json README.md
Change Summary:
Adding .gitignore and putting config.json there
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+257) |
-
Tool: Pyflakes Ignored Files: git_repository.go repository.go .gitignore main.go web_service.go util.go README.md Tool: PEP8 Style Checker Ignored Files: git_repository.go repository.go .gitignore main.go web_service.go util.go README.md
-
-
README.md (Diff revision 4) Just a nit pick, but underlines with
=
and-
in Markdown should be the same length as the text they are underlining. Same below.
Change Summary:
Updated description
Description: |
|
---|
Change Summary:
Adding install instructions to README
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+282) |
-
Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md
-
-
-
web_service.go (Diff revision 5) It might be nice to log the URLs that are accessed and the resulting response status/payload size (kind of an access log).
-
Change Summary:
Added access log and fixed David's suggestions.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+311) |
-
Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md
-
-
git_repository.go (Diff revision 6) As I read through this, I'm wondering if instead of logging, we should make this function return the error (use the typical data, err pattern of returns that go uses). That way we can have a single log statement in the code that calls this method.
-
main.go (Diff revision 6) This is pretty verbose. Can we just have a single line in the log? (get rid of the =====s)
-
main.go (Diff revision 6) To be honest, this comment doesn't really add anything useful (the method name is quite self-explanatory).
-
util.go (Diff revision 6) Does this need to be exported? It looks like we don't use it outside of this file.
That said, it might be nice to change this so that LoadConfig returns this instead of storing it as a global.
Change Summary:
Minor code refactoring
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+312) |
-
Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md
-
-
web_service.go (Diff revision 7) These cases can probably be combined (we expect either an err or a blob)
Change Summary:
Slight refactoring (to fix David's issue)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+309) |
-
Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md
Change Summary:
Added a GetPath to web_service.go
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+331) |
-
Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md
Change Summary:
Modified GET path to account for /info/ref that RB adds to the repository GET request
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+332) |
-
Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go .gitignore main.go web_service.go util.go README.md
Change Summary:
Added Basic Auth, port value to configuration, and some clean up
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 11 (+468) |
-
Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md
Change Summary:
Getting file blob and checking file existance based on commit id, file path pairing (in addition to already supported revision id)
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 12 (+567) |
-
Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md
Change Summary:
Updated description / testing done a bit / removed WIP
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
-
This is looking really solid!
Can you go through the comments for things and make sure that they follow these conventions: http://blog.golang.org/godoc-documenting-go-code ?
-
git_repository.go (Diff revision 12) Instead of grabbing github master, how about using "gopkg.in/libgit2/git2go.v22"? That way people building this can just do
go get
to grab the dependency rather than the complicated git/make/etc steps. -
main.go (Diff revision 12) These two comments don't really add very much (it's pretty easy to find these functions in a relatively small codebase, and other than that they just repeat the function name)
-
-
-
-
session.go (Diff revision 12) This doesn't seem to provide any advantage over just calling json.Marshal(session) directly.
Change Summary:
Changes to comments and naming conventions, to be more Go-ish.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+591) |
-
Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md
-
-
-
util.go (Diff revision 13) You have an "scm" field in the repositories in the config. That seems like a more reliable way of detecting the repository type.
Change Summary:
Minor changes to sample_config.json (Added space afer colon)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+591) |
-
Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md
Change Summary:
Added SCM types back to sample_config.json
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+595) |
-
Tool: Pyflakes Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md Tool: PEP8 Style Checker Ignored Files: git_repository.go sample_config.json repository.go auth.go .gitignore main.go session.go routes.go util.go README.md