Review Board Gateway hosting service integration

Review Request #6850 — Created Jan. 31, 2015 and submitted

Information

Review Board
master
6878be8...

Reviewers

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

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


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.

brenniebrennie

Col: 6 W292 no newline at end of file

reviewbotreviewbot

'is_release' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

daviddavid

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

reviewbotreviewbot

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

reviewbotreviewbot

'six' imported but unused

reviewbotreviewbot

'URLError' imported but unused

reviewbotreviewbot

Col: 32 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

undefined name 'ugettext'

reviewbotreviewbot

undefined name 'ugettext'

reviewbotreviewbot

undefined name 'ugettext'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

This should end in a period.

daviddavid

Can we stick this in an else clause?

daviddavid

We should log unexpected errors.

daviddavid

We should log unexpected errors.

daviddavid

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

reviewbotreviewbot

undefined name 'url'

reviewbotreviewbot

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

reviewbotreviewbot

This isn't correct.

daviddavid

This should probably be "Review Board Gateway"

daviddavid

Checks -> Check

daviddavid

Authorizes -> Authorize.

daviddavid

Determines -> Determine

daviddavid

Gets -> Get

daviddavid

Checks -> Check

daviddavid

This should be "Get" rather than "Gets"

daviddavid

The summary part of the docstring should fit on one line. It should also be written using the imperative mood …

daviddavid

Same comments here.

daviddavid

Returns -> Return

daviddavid

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

reviewbotreviewbot

Can you add a docstring for this class?

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

I'm not sure we really care to log 404s, either.

daviddavid

We definitely don't care about 404s here.

daviddavid

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Surround this with parenthesis and you won't need the backslashes. Also, you should be using the bytestring literal (b'') because …

brenniebrennie

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

reviewbotreviewbot

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

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

reviewbotreviewbot

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

ChesterChester

I would recommend if not start:

ChesterChester

You could add one more parameter raw_content into the definition, when it is False, return data, headers directly, otherwise return …

ChesterChester

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

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 1)
     
     
    Show all issues
    Col: 6
     W292 no newline at end of file
    
  3. setup.py (Diff revision 1)
     
     
    Show all issues
     'is_release' imported but unused
    
  4. setup.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. 
      
JY
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
  2. 
      
david
  1. 
      
  2. setup.py (Diff revision 2)
     
     
     
    Show all issues

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

  3. 
      
JY
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
  2. setup.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
JY
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
  2. setup.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
JY
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        setup.py
        reviewboard/hostingsvcs/rbgateway.py
    
    
  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
     
     
    Show all issues
     'six' imported but unused
    
  3. reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
     
     
    Show all issues
     'URLError' imported but unused
    
  4. reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
     
     
    Show all issues
    Col: 32
     E226 missing whitespace around arithmetic operator
    
  5. reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
     
     
    Show all issues
     undefined name 'ugettext'
    
  6. reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
     
     
    Show all issues
     undefined name 'ugettext'
    
  7. reviewboard/hostingsvcs/rbgateway.py (Diff revision 5)
     
     
    Show all issues
     undefined name 'ugettext'
    
  8. setup.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  9. 
      
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
JY
david
  1. Just a few very simple comments.

  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 7)
     
     
    Show all issues

    This should end in a period.

  3. reviewboard/hostingsvcs/rbgateway.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues

    Can we stick this in an else clause?

  4. reviewboard/hostingsvcs/rbgateway.py (Diff revision 7)
     
     
     
    Show all issues

    We should log unexpected errors.

  5. reviewboard/hostingsvcs/rbgateway.py (Diff revision 7)
     
     
     
    Show all issues

    We should log unexpected errors.

  6. 
      
JY
reviewbot
  1. 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
    
    
  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 8)
     
     
    Show all issues
     undefined name 'url'
    
  3. setup.py (Diff revision 8)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. 
      
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 9)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
david
  1. 
      
  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    This isn't correct.

  3. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    This should probably be "Review Board Gateway"

  4. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    Checks -> Check

    1. 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?

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

    3. Ok will do!

  5. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    Authorizes -> Authorize.

  6. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    Determines -> Determine

  7. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    Gets -> Get

  8. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    Checks -> Check

  9. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    This should be "Get" rather than "Gets"

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

  11. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
     
    Show all issues

    Same comments here.

  12. reviewboard/hostingsvcs/rbgateway.py (Diff revision 9)
     
     
    Show all issues

    Returns -> Return

  13. 
      
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 10)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
brennie
  1. 
      
  2. Show all issues

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

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

  3. 
      
david
  1. 
      
  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)
     
     
    Show all issues

    Can you add a docstring for this class?

  3. reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)
     
     
    Show all issues

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

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

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

  6. reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)
     
     
     
    Show all issues

    I'm not sure we really care to log 404s, either.

  7. reviewboard/hostingsvcs/rbgateway.py (Diff revision 10)
     
     
     
    Show all issues

    We definitely don't care about 404s here.

  8. 
      
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 11)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
JY
reviewbot
  1. 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
    
    
  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 12)
     
     
    Show all issues
    Col: 19
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/hostingsvcs/rbgateway.py (Diff revision 12)
     
     
    Show all issues
    Col: 19
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/hostingsvcs/rbgateway.py (Diff revision 12)
     
     
    Show all issues
    Col: 15
     E128 continuation line under-indented for visual indent
    
  5. setup.py (Diff revision 12)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  6. 
      
JY
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 13)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 14)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
brennie
  1. <p>Looks good to me!</p>

  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 14)
     
     
     
    Show all issues

    Blank line between these.

  3. reviewboard/hostingsvcs/rbgateway.py (Diff revision 14)
     
     
     
    Show all issues

    Blank line between these.

  4. reviewboard/hostingsvcs/tests.py (Diff revision 14)
     
     
     
    Show all issues

    Blank line between these.

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

  6. 
      
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
brennie
  1. Sorry, two little nits I missed last time around, and then I'm comfortable with landing this.

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

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

  2. reviewboard/hostingsvcs/rbgateway.py (Diff revision 15)
     
     
     
    Show all issues

    Blank line between these.

  3. reviewboard/hostingsvcs/rbgateway.py (Diff revision 15)
     
     
     
    Show all issues

    Blank line between these.

  4. 
      
JY
reviewbot
  1. 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
    
    
  2. setup.py (Diff revision 16)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
JY
Review request changed
Status:
Completed
Change Summary:
Pushed to master (2060a5d)
Chester
  1. 
      
  2. 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.

  3. reviewboard/hostingsvcs/rbgateway.py (Diff revision 16)
     
     
    Show all issues

    I would recommend if not start:

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

    2. sorry I meant if start. Your implementation makes sense.

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

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

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

  5.