• 
      

    rb-gateway

    Review Request #6831 — Created Jan. 25, 2015 and submitted

    Information

    rb-gateway
    master
    ba391ab...

    Reviewers

    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 session

    Local 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-gateway

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

    daviddavid

    Can you add a default case to log an error?

    daviddavid

    How about storing the "Repos" var as a map from name to repo?

    daviddavid

    These should actually be the same URL (and the same handler). Inside the handler, we can switch between "get file" …

    daviddavid

    Just a nit pick, but underlines with = and - in Markdown should be the same length as the text …

    brenniebrennie

    Can we call this FileExists? (remove the "Get")

    daviddavid

    It might be nice to log the URLs that are accessed and the resulting response status/payload size (kind of an …

    daviddavid

    These two lines could be combined (if repo.GetFileExists(id) {)

    daviddavid

    As I read through this, I'm wondering if instead of logging, we should make this function return the error (use …

    daviddavid

    This is pretty verbose. Can we just have a single line in the log? (get rid of the =====s)

    daviddavid

    To be honest, this comment doesn't really add anything useful (the method name is quite self-explanatory).

    daviddavid

    Does this need to be exported? It looks like we don't use it outside of this file. That said, it …

    daviddavid

    These cases can probably be combined (we expect either an err or a blob)

    daviddavid

    Instead of grabbing github master, how about using "gopkg.in/libgit2/git2go.v22"? That way people building this can just do go get to …

    daviddavid

    These two comments don't really add very much (it's pretty easy to find these functions in a relatively small codebase, …

    daviddavid

    This doesn't need to be exported out of this file.

    daviddavid

    This doesn't need to be exported out of this file.

    daviddavid

    This doesn't need to be exported out of this file.

    daviddavid

    This doesn't seem to provide any advantage over just calling json.Marshal(session) directly.

    daviddavid

    Can you make sure that there's always a space after a colon?

    daviddavid

    You have an "scm" field in the repositories in the config. That seems like a more reliable way of detecting …

    daviddavid
    reviewbot
    1. Tool: Pyflakes
      Ignored Files:
          git_repository.go
          repository.go
          main.go
          web_service.go
          util.go
          config.json
      
      
      
      Tool: PEP8 Style Checker
      Ignored Files:
          git_repository.go
          repository.go
          main.go
          web_service.go
          util.go
          config.json
      
      
    2. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    david
    1. 
        
    2. git_repository.go (Diff revision 2)
       
       
      Show all issues

      Can you do this?

      cmd := exec.Command("git", "show", id)
      cmd.Dir = repo.Path
      ...
      

      Same above.

      1. I may use git2go as Barret suggested since executing shell commands are generally unsafe.

    3. util.go (Diff revision 2)
       
       
      Show all issues

      Can you add a default case to log an error?

    4. util.go (Diff revision 2)
       
       
       
       
       
       
      Show all issues

      How about storing the "Repos" var as a map from name to repo?

    5. web_service.go (Diff revision 2)
       
       
       
      Show all issues

      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.

    6. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    brennie
    1. 
        
    2. README.md (Diff revision 4)
       
       
      Show all issues

      Just a nit pick, but underlines with = and - in Markdown should be the same length as the text they are underlining. Same below.

      1. With the font I'm using in Sublime, they are the same length :( Is there a standard font when it comes to these things?

      2. Switch to any Monospaced font.

    3. 
        
    JY
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    david
    1. 
        
    2. git_repository.go (Diff revision 5)
       
       
      Show all issues

      Can we call this FileExists? (remove the "Get")

    3. web_service.go (Diff revision 5)
       
       
      Show all issues

      It might be nice to log the URLs that are accessed and the resulting response status/payload size (kind of an access log).

    4. web_service.go (Diff revision 5)
       
       
       
      Show all issues

      These two lines could be combined (if repo.GetFileExists(id) {)

    5. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    david
    1. 
        
    2. git_repository.go (Diff revision 6)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      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.

    3. main.go (Diff revision 6)
       
       
       
       
      Show all issues

      This is pretty verbose. Can we just have a single line in the log? (get rid of the =====s)

    4. main.go (Diff revision 6)
       
       
      Show all issues

      To be honest, this comment doesn't really add anything useful (the method name is quite self-explanatory).

    5. util.go (Diff revision 6)
       
       
      Show all issues

      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.

    6. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    david
    1. 
        
    2. web_service.go (Diff revision 7)
       
       
       
       
       
       
       
       
       
      Show all issues

      These cases can probably be combined (we expect either an err or a blob)

    3. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    JY
    david
    1. 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 ?

      1. I added a comment to every exported function and made sure the comment started with the function name. Also un-capitalized consts and some things that didn't need to be exported. Hopefully didn't miss anything.

    2. git_repository.go (Diff revision 12)
       
       
      Show all issues

      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.

    3. main.go (Diff revision 12)
       
       
       
       
       
      Show all issues

      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)

    4. routes.go (Diff revision 12)
       
       
      Show all issues

      This doesn't need to be exported out of this file.

    5. routes.go (Diff revision 12)
       
       
      Show all issues

      This doesn't need to be exported out of this file.

    6. routes.go (Diff revision 12)
       
       
      Show all issues

      This doesn't need to be exported out of this file.

    7. session.go (Diff revision 12)
       
       
       
       
       
       
       
       
      Show all issues

      This doesn't seem to provide any advantage over just calling json.Marshal(session) directly.

    8. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    david
    1. 
        
    2. sample_config.json (Diff revision 13)
       
       
       
       
       
       
       
       
      Show all issues

      Can you make sure that there's always a space after a colon?

    3. util.go (Diff revision 13)
       
       
       
       
       
       
       
       
      Show all issues

      You have an "scm" field in the repositories in the config. That seems like a more reliable way of detecting the repository type.

      1. I actually removed the scm field from config.json, I forgot to update sample_config.json. The scm field isn't being used.

        My local config.json looks like this:
        {
        "port":8002,
        "username":"myuser",
        "password":"mypass",
        "repositories":[
        {"name":"testrepo", "path":"/home/j/git-sandbox/.git"}
        ]
        }

        I was thinking without it, it would be more reliable to verify that the path points to a SCM controlled repository (ensuring it ends in .git and what not). I'm not too sure, which do you prefer?

      2. I think I prefer having it explicitly listed. Not every SCM is as easily discernible from the filename.

    4. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    JY
    reviewbot
    1. 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
      
      
    2. 
        
    david
    1. This looks great to me. Is there anything else you wanted to change before I land this patch?

      1. I don't think so. I'll work on the API's for the post commit UIs next, but I'll open another rr for that. :)

    2. 
        
    JY
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (23beb03)