Add support for Gerrit as a hosting service

Review Request #8938 - Created May 10, 2017 and updated

Barret Rennie
Review Board
release-2.5.x
8923, 8913
132ec30...
reviewboard

This patch adds support for Gerrit as a hosting service when the Gerrit
server has the gerrit-reviewboard plugin installed (see: /r/8919/ et
al.). The Gerrit API does not provide sufficient information (e.g., it
cannot retrieve files by blob ID, cannot list commits, and cannot
retrieve diffs for arbitrary commits not related to changes), so we rely
on the plugin for this. This hosting service will check for the plugin's
presence via the Gerrit API and will not allow repositories without it
installed.

  • Tested post-commit review request creation.
  • Tested rbt post with a Gerrit-backed repository.
  • Ran unit tests.
Loading file attachments...

  • 20
  • 0
  • 56
  • 16
  • 92
Description From Last Updated
This should go after all the django.utils.six imports David Trowbridge David Trowbridge
Use parens instead of \ David Trowbridge David Trowbridge
Trailing comma isn't necessary since these are just args. ) can be on this line too. David Trowbridge David Trowbridge
Should be marked for translation. David Trowbridge David Trowbridge
Should be marked for translation. David Trowbridge David Trowbridge
Should end in a period to match the others. David Trowbridge David Trowbridge
Instead of implementing a custom clean method you should be able to just pass in min_value and max_value to the ... David Trowbridge David Trowbridge
Other implementations call this arg hosting_service David Trowbridge David Trowbridge
typo: ot David Trowbridge David Trowbridge
Comma after "Otherwise" David Trowbridge David Trowbridge
The string included in the comment here doesn't match the code. David Trowbridge David Trowbridge
The implementation of _parse_plugin_version could raise exceptions if we get something unexpected from the server. Can we try/catch and convert ... David Trowbridge David Trowbridge
Blank line after this. David Trowbridge David Trowbridge
Add two blank lines after this. David Trowbridge David Trowbridge
Undo this change. Maybe wrap the whole thing in parens too? David Trowbridge David Trowbridge
Undo this change. David Trowbridge David Trowbridge
Undo this change. David Trowbridge David Trowbridge
Undo this change. David Trowbridge David Trowbridge
Undo this change. David Trowbridge David Trowbridge
Add two blank lines before this. David Trowbridge David Trowbridge
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

Barret Rennie
Review request changed

Change Summary:

Unit tests.

Summary:

-WIP: Gerrit Support
+Add support for Gerrit as a hosting service

Description:

~  

WIP

  ~

This patch adds support for Gerrit as a hosting service when the Gerrit

  + server has the gerrit-reviewboard plugin installed (see: /r/8919/ et
  + al.). The Gerrit API does not provide sufficient information (e.g., it
  + cannot retrieve files by blob ID, cannot list commits, and cannot
  + retrieve diffs for arbitrary commits not related to changes), so we rely
  + on the plugin for this. This hosting service will check for the plugin's
  + presence via the Gerrit API and will not allow repositories without it
  + installed.

Testing Done:

  +
  • Tested post-commit review request creation.
  +
  • Tested rbt post with a Gerrit-backed repository.
  +
  • Ran unit tests.

Diff:

Revision 2 (+1044 -13)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
Barret Rennie
David Trowbridge
  1. 
      
  2. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     

    t comes after s.

  3. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     

    Wrap in parens instead of using the continuation character.

    1. I discovered pycharm auto imports while doing the java stuff but it doesnt do it so pretty.

  4. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Wrap this error in _()?

  5. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    typo: ot

  6. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     
     
     

    e should be passed in as another arg, since the logger functions do their own format operation.

  7. Can you add two blank lines between these?

  8. Undo this change.

  9. Undo this change.

  10. Undo this change.

  11. Can you add two blank lines between these?

  12. 
      
Christian Hammond
  1. 
      
  2. reviewboard/hostingsvcs/forms.py (Diff revision 3)
     
     

    "subclasses"

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

    I think you meant "hosting account"?

  4. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    The Meta class should come last in the form.

  5. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    The &gt; can be inside the <tt>. In other words, one <tt> should be fine.

  6. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Here too.

  7. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Missing : after :py:meth.

  8. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    "hostingsvcs"

  9. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    "project form?"

  10. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Seems like this should maybe be "Gerrit URL"?

  11. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    This is missing help text.

  12. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    This should be "Project name".

    It's also missing help text.

  13. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    The error should be less terse. Maybe "This is not a valid port. Ports must be in the range of 0-65535."

  14. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Might read better as "The only required part is the FQDN ...

  15. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     

    Looks like this could be one line?

  16. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     

    This can be one statement.

  17. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    What happens if this changes in the future? Think we should code defensively here. If the data doesn't include this, log it and return the data as-is. Might also want to make the \n optional, as that seems like an easy thing that can change.

  18. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     

    We only use this in one error message that's only called when configuring a repository, so let's do that there instead.

  19. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Probably belongs up with form.

  20. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    This is the default.

  21. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     

    You don't have to provide this argument. It will be captured by kwargs.

  22. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     

    No need to provide this either.

  23. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Missing , optional.

  24. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Missing , optional.

  25. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Needs the full class path.

  26. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Ideally we should flesh this out. "Unable to authenticate with Gerrit. The username or password might be invalid."

    It's best to avoid overly terse errors that look like they came from Posix libraries.

  27. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Missing docs.

  28. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    We should be using encrypt_password and decrypt_password for any credentials being stored.

  29. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Typo: "reviewbaord" -> "reviewboard"

  30. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     
     
     
     

    I'd suggest following GitHub's model and defining an api_get that does the common stuff (passing credentials and only returning the data).

  31. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    I am left in suspense.

  32. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Historically, this function comes before the rest of the overriden functions (this, then authorize, then the rest).

  33. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     

    Typo: "reviewbaord" -> "reviewboard"

  34. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     

    You can do:

    parts = [
        ...
    ] + rest_parts
    
  35. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     
     
     

    Better formatted as:

    url = ('%s/'
           % urljoin(...))
    

    I believe that will fit on two lines. Maybe three, if you need to put '/'.join(parts) on the next line.

  36. reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 3)
     
     
     
     

    Should be one statement.

  37. Undo this change. The .closest is part of the whole selector process. The .setVisible is the thing we're calling on the resulting elements.

    1. This is in contravention to basically the rest of our JS style.

    2. I think this would be the best:

      $twoFactorAuthRows = $authForm
          .find('[data-required-for-2fa]')
          .closest('.form-row')
              .setVisible(hostingInfo.needs_two_factor_auth_code);
      
  38. 
      
Christian Hammond
  1. 
      
  2. reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
     
     
     
     
     
     

    This can only be done if we're shipping this with 3.0. We still support Python 2.6 for Review Board 2.5.

  3. 
      
Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
Review request changed
David Trowbridge
  1. 
      
  2. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    This should go after all the django.utils.six imports

  3. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     
     

    Use parens instead of \

  4. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Trailing comma isn't necessary since these are just args. ) can be on this line too.

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

    Should be marked for translation.

  6. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     
     

    Should be marked for translation.

  7. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Should end in a period to match the others.

  8. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Instead of implementing a custom clean method you should be able to just pass in min_value and max_value to the forms.IntegerField constructor.

  9. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Other implementations call this arg hosting_service

  10. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    typo: ot

  11. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Comma after "Otherwise"

  12. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    The string included in the comment here doesn't match the code.

  13. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Does this need a try/catch?

  14. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     
     

    The implementation of _parse_plugin_version could raise exceptions if we get something unexpected from the server. Can we try/catch and convert to a RepositoryError?

  15. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Does this need a try/catch?

  16. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Blank line after this.

  17. reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
     
     

    Does this need a try/catch?

  18. Add two blank lines after this.

  19. Undo this change. Maybe wrap the whole thing in parens too?

  20. reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 5)
     
     
     
     

    Undo this change.

  21. Undo this change.

  22. Undo this change.

  23. Undo this change.

  24. Add two blank lines before this.

  25. 
      
Loading...