Add support for Gerrit as a hosting service

Review Request #8938 — Created May 10, 2017 and submitted

brennie
Review Board
release-2.5.x
8923, 8913
f04d061...
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...

  • 0
  • 0
  • 145
  • 23
  • 168
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
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

brennie
brennie
david
  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. 
      
chipx86
  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. 
      
chipx86
  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. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  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. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
david
  1. 
      
  2. reviewboard/hostingsvcs/gerrit.py (Diff revision 8)
     
     

    We should probably assert that gerrit_url is not None here.

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

    Leftover debug output?

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

    Leftover debug output?

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

    I think this would be more readable if we split it up a bit:

    url = urljoin(repository.extra_data['gerrit_url'],
                  '/'.join(parts))
    
    if query:
        url = '%s/?%s' % (url, urlencode(query))
    else:
        url = '%s/' % url
    
  6. reviewboard/hostingsvcs/github.py (Diff revision 8)
     
     

    Why this change?

  7. 
      
brennie
david
  1. 
      
  2. reviewboard/hostingsvcs/gerrit.py (Diff revision 9)
     
     

    Blank line after this.

  3. So in python 3, json.dumps requires that all the keys and values be str. I think instead of having a bunch of bytestrings in here, we should have them all be unicode, but then do json.dumps(...).encode('utf-8')

  4. Blank line after this.

  5. 
      
brennie
Review request changed

Change Summary:

Addressed David's feedback.

Commit:

-639c85ab2d5a1df87bfe49e9aa0814c90c1618d3
+2049fb6d13a537310bd7fba1dc37c32d24bfba50

Diff:

Revision 10 (+1215 -11)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  1. 
      
  2. reviewboard/hostingsvcs/gerrit.py (Diff revision 11)
     
     
     
     

    Because we know the format, this would be a lot more readable as:

    'required': '%d.%d.%d' % self.REQUIRED_PLUGIN_VERSION,
    
  3. reviewboard/hostingsvcs/gerrit.py (Diff revision 11)
     
     

    Please pass in e as an arg to logging.error instead of pre-formatting it.

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

    This should fit on one line.

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

    This can just use + instead of extend

    1. Not if its a tuple.

    2. >>> (1, 2, 3) + (4, 5)
      (1, 2, 3, 4, 5)
      
    3. Oh, I see, you're appending a tuple to a list. Nevermind then.

  6. 
      
brennie
Review request changed

Change Summary:

Addressed David's feedback.

Commit:

-281ec69a84c890149fe2c5f6562f5a9ce7cbc3d5
+d7ca381374c4239518f524add0f7bfa44f9d2a80

Diff:

Revision 12 (+1211 -11)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  1. Looks good. Please re-run manual and automated tests before pushing.

  2. 
      
chipx86
  1. This looks like more feedback than it is. Primarily some small docs/error message stuff repeated for different instances.

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

    Swap these.

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

    Should be in alphabetical order.

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

    I know the base implementation doesn't do anything, but it might in the future. Let's call the parent method here.

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

    "to the"

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

    We should specify the URL here (first argument in *args), since we otherwise don't have enough information to figure out what went wrong.

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

    Can we have a module-level logger?

    1. I thought we couldn't use module-level loggers for some reason?

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

    "Source code"

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

    Mind linking to the URL here?

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

    "Gerrit"

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

    The ] shouldn't be here.

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

    We should link to the URL for the repository in this message. We can then provide instructions in that repository's README for installation instructions.

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

    We should specify the repository ID here, since we otherwise don't have enough information to debug problems.

    1. We don't have a repository ID here, unless I am mistaken.

    2. You're right, we don't. We can include the Gerrit URL in this case.

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

    Keyword arguments should ideally be used in localized text when there's more than one variable being injected. Helps with translations when the order ends up changing.

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

    We should specify the URL here, since we otherwise don't have enough information to debug problems.

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

    Missing gettext.

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

    Missing gettext.

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

    Parens around the equality check.

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

    Missing gettext.

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

    Missing gettext.

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

    gettext

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

    Can we catch an exception here and handle it gracefully, in case something goes wonky with the response?

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

    We should specify the repository ID here, since we otherwise don't have enough information to debug problems.

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

    We should ideally use keywords for the format string here.

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

    Can you place arguments starting right after Commit(? These aren't long enough to require wrapping.

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

    Should use +=. It's faster. Or just + on the previous line.

    1. Can't if rest_parts is a tuple, like I mentioned in a previous review.

  27. reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 13)
     
     
     

    Alphabetical order.

  28. reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Private methods go last.

  29. reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 13)
     
     
     
     
     

    You can start after Repository(

  30. This shouldn't be necessary on Gerrit.

    Same with other calls in this file.

  31. The parens aren't necessary in JavaScript.

  32. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

chipx86
  1. 
      
  2. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    No need for the backtick when using a reference without a space in the name.

  3. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     
     

    This can fit on one line.

  4. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    The dictionary should be expanded.

  5. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    Same here.

  6. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    The dictionary should be expanded.

  7. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    Same here.

  8. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    Same here.

  9. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    Same here.

  10. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    Same here.

  11. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    Same here.

  12. reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
     
     

    Same here.

  13. reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revisions 13 - 16)
     
     
     
     
     

    Alphabetical order.

  14. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/gerrit.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     

    I just realized this isn't going to work. e.read() makes no sense for a b64decode. The error isn't really descriptive of the problem, either. Can you fix this up and add a unit test for it?

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

    This needs to be unfolded as well.

  4. 
      
brennie
brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (308de19)
Loading...