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

  • 37
  • 16
  • 14
  • 67
Description From Last Updated
"subclasses" Christian Hammond Christian Hammond
I think you meant "hosting account"? Christian Hammond Christian Hammond
The Meta class should come last in the form. Christian Hammond Christian Hammond
The &gt; can be inside the <tt>. In other words, one <tt> should be fine. Christian Hammond Christian Hammond
Here too. Christian Hammond Christian Hammond
Missing : after :py:meth. Christian Hammond Christian Hammond
"hostingsvcs" Christian Hammond Christian Hammond
"project form?" Christian Hammond Christian Hammond
Seems like this should maybe be "Gerrit URL"? Christian Hammond Christian Hammond
This is missing help text. Christian Hammond Christian Hammond
This should be "Project name". It's also missing help text. Christian Hammond Christian Hammond
The error should be less terse. Maybe "This is not a valid port. Ports must be in the range of ... Christian Hammond Christian Hammond
Might read better as "The only required part is the FQDN ... Christian Hammond Christian Hammond
Looks like this could be one line? Christian Hammond Christian Hammond
This can be one statement. Christian Hammond Christian Hammond
What happens if this changes in the future? Think we should code defensively here. If the data doesn't include this, ... Christian Hammond Christian Hammond
We only use this in one error message that's only called when configuring a repository, so let's do that there ... Christian Hammond Christian Hammond
Probably belongs up with form. Christian Hammond Christian Hammond
This is the default. Christian Hammond Christian Hammond
You don't have to provide this argument. It will be captured by kwargs. Christian Hammond Christian Hammond
No need to provide this either. Christian Hammond Christian Hammond
Missing , optional. Christian Hammond Christian Hammond
Missing , optional. Christian Hammond Christian Hammond
Needs the full class path. Christian Hammond Christian Hammond
Ideally we should flesh this out. "Unable to authenticate with Gerrit. The username or password might be invalid." It's best ... Christian Hammond Christian Hammond
Missing docs. Christian Hammond Christian Hammond
We should be using encrypt_password and decrypt_password for any credentials being stored. Christian Hammond Christian Hammond
Typo: "reviewbaord" -> "reviewboard" Christian Hammond Christian Hammond
This can only be done if we're shipping this with 3.0. We still support Python 2.6 for Review Board 2.5. Christian Hammond Christian Hammond
I'd suggest following GitHub's model and defining an api_get that does the common stuff (passing credentials and only returning the ... Christian Hammond Christian Hammond
I am left in suspense. Christian Hammond Christian Hammond
Historically, this function comes before the rest of the overriden functions (this, then authorize, then the rest). Christian Hammond Christian Hammond
Typo: "reviewbaord" -> "reviewboard" Christian Hammond Christian Hammond
You can do: parts = [ ... ] + rest_parts Christian Hammond Christian Hammond
Better formatted as: url = ('%s/' % urljoin(...)) I believe that will fit on two lines. Maybe three, if you ... Christian Hammond Christian Hammond
Should be one statement. Christian Hammond Christian Hammond
Undo this change. The .closest is part of the whole selector process. The .setVisible is the thing we're calling on ... Christian Hammond Christian Hammond
Barret Rennie
Barret Rennie
Barret Rennie
Review request changed
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?

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.

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.

Loading...