Add support for GitLab API v4

Review Request #9383 - Created Dec. 13, 2017 and updated

Barret Rennie
Review Board
release-2.5.x
eff43f2...
reviewboard

This patch adds support for GitLab API v4 while maintaining backwards
compatability with v3. When we don't know the API version of the host,
we will determine it by optimistically assuming v4 and then falling back
to v3.

Additionally, we no longer use the user's password to retrieve an API
token from GitLab. Newer versions (9+) remove the /session API and
require users to generate a token per application in their profile
settings. We now accept this API token directly with instructions how to
create it on new and old versions of GitLab.

Some other minor changes were made:

  • We now log full stack traces from unexpected Exceptions in the
    Repository Form
  • We now have commit summaries for post commit review for older versions
    of GitLab that don't include the entire commit message
  • Support for nested groups in newer versions of GitLab
  • Linked a gitlab.com nested group repository (e.g. group foo/bar, repo
    baz) and was able to do post-commit review.
  • Linked a GitLab 7 group and user repository and was able to do
    post-commit review for both.
  • Ran unit tests.
Loading file attachments...

  • 45
  • 0
  • 57
  • 1
  • 103
Description From Last Updated
GitLab API tests should continue to test API v3, with additional tests for API v4, rather than just moving them ... Christian Hammond Christian Hammond
Make sure you build the docs and check for new errors. There are lots of syntax and reference errors throughout ... Christian Hammond Christian Hammond
You should be able to just pass self.causes into the %r directly. It'll repr the list, which will then auto-repr ... Christian Hammond Christian Hammond
Help text shouldn't use <br>. One long string is better here. Christian Hammond Christian Hammond
Should use double-backticks, since this is rst and not markdown. David Trowbridge David Trowbridge
, unused after the type. Christian Hammond Christian Hammond
Other docs don't do "Raised if..." That's implied and can be left off. For the last one, "occurred" would be ... Christian Hammond Christian Hammond
"SHA1", and of the blob for the file. Christian Hammond Christian Hammond
"SHA1" Christian Hammond Christian Hammond
, unused Christian Hammond Christian Hammond
Same comment about the "Raised if ..." Christian Hammond Christian Hammond
Same notes as above. Christian Hammond Christian Hammond
Same notes as above about "Raised if ..." Christian Hammond Christian Hammond
While you're here, this should be "Return a list of branches." Christian Hammond Christian Hammond
This isn't correct. Christian Hammond Christian Hammond
Same notes as above. Christian Hammond Christian Hammond
This isn't correct. Shoud be reviewboard.scmtools.core.Commit. Christian Hammond Christian Hammond
Same notes as above with "Raised if ..." Christian Hammond Christian Hammond
reviewboard.scmtools.core.Commit Christian Hammond Christian Hammond
Same notes as above with "Raised if ..." Christian Hammond Christian Hammond
Can you include the comment from above here as well, about why we're checking both message and title? Christian Hammond Christian Hammond
Same notes about "Raised if ..." Christian Hammond Christian Hammond
I assume it's checking both for GitLab API version differences? Can you document those here? Christian Hammond Christian Hammond
While here, can you add a blank line between these? Christian Hammond Christian Hammond
Same notes about "Raised if ..." Christian Hammond Christian Hammond
Same here. Christian Hammond Christian Hammond
Same here. Christian Hammond Christian Hammond
:samp: Christian Hammond Christian Hammond
Will the old version work on the new API? Christian Hammond Christian Hammond
Typo: "reviewboard" Christian Hammond Christian Hammond
Same note about "Raised if ..." Christian Hammond Christian Hammond
Here too. Christian Hammond Christian Hammond
Same notes about "Raised if ..." Christian Hammond Christian Hammond
This is a pretty weird and hard-to-read statement, particularly with all the layers of nots and the ^. Given that ... Christian Hammond Christian Hammond
Same comments about "Raised if ..." Christian Hammond Christian Hammond
"Return the version ..." Christian Hammond Christian Hammond
Syntax error. Missing the backtick. Christian Hammond Christian Hammond
I don't see how we'd ever be in a case where the or would occur. If we hit an exception, ... Christian Hammond Christian Hammond
Typo: "arguments" Christian Hammond Christian Hammond
Same notes about the "Raised if ..." Christian Hammond Christian Hammond
This should never happen. If it does, it's an internal failure. We should assert. Christian Hammond Christian Hammond
You can use basestring. Christian Hammond Christian Hammond
You can use basestring. Christian Hammond Christian Hammond
I might be wrong about some part of this, but according urlopen docs, the first argument to urlopen is url, ... Christian Hammond Christian Hammond
assertIsNone Christian Hammond Christian Hammond
David Trowbridge
  1. 
      
  2. Typo in description: GitHub -> GitLab

    You also have what looks like was supposed to be bullet points but isn't, because wrapping happened.

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
     

    j before l

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Missing a format specifier in here for message?

  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    typo: argsuments

  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Should have a trailing comma

  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Mind adding args/raises to this docstring?

  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Mind adding args/returns/raises?

  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Mind adding args/returns/raises?

  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Mind adding args/returns/raises?

  11. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Mind adding args/returns/raises?

  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    gitlab -> GitLab

    Should we use double backticks to be consistent with other documentation?

  13. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Mind adding args/returns/raises?

  14. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Mind adding returns/raises?

  15. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Typo: Retur

    Mind adding returns/raises?

  16. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Docstring?

  17. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Mind adding args/returns/raises?

  18. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    returns -> return

  19. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Otherwise what?

  20. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    returns -> return

  21. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Docstring?

  22. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Header keys should be bytestrings

  23. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Docstring?

  24. reviewboard/scmtools/forms.py (Diff revision 1)
     
     

    Should we add some extra context to this log message?

  25. 
      
Barret Rennie
David Trowbridge
  1. 
      
  2. Typo in description: "Newer version" -> "Newer versions"

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     

    This should likely be __repr__

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     

    Still missing real docstring content here.

  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     

    typo: succesfully -> successfully

  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     

    Too many blank lines.

  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     
     

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     
     

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     
     

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     
     

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

  11. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     
     

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     
     

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

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

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

  14. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     
     

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

  15. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     
     

    HTTPError is a subclass of URLError, and URLError comes from urllib2.

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

    typo: __repr_ -> __repr__

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

    Should be two lines.

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

    Typo: Return -> Returns

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

    Needs Returns

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

    This seems like a bad log message.

  7. 
      
Barret Rennie
David Trowbridge
  1. 
      
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     

    There's an extra space after "Personal Access" that shouldn't be there.

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

    These two lines could be one.

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     

    Gitlab -> GitLab

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

    typo: acount

  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     
     
     

    How about wrapping this as:

    return self._api_get(self.account.hosting_url,
                         'groups/%s' % group_id)[0]
    
  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     

    Same here re: wrapping.

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

    Header keys should be binary.

  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     
     

    There's an extra comma at the end of this line, and the ) shouldn't be on the next line.

  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     
     

    Closing ) from function call shouldn't be on its own line, plus there's an extra comma after the last arg.

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

    Trailing ) for function calls shouldn't be on its own line.

  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Indentation got messed up here.

  13. Closing ) for function call shouldn't be on its own line. There's also an extra , after the last arg.

  14. Needs a trailing comma.

  15. Needs a trailing comma.

  16. Needs a trailing comma.

  17. 
      
Barret Rennie
Review request changed

Change Summary:

Addressed David's issues.

Commit:

-c48f6e0ad71c1e68ffd2e2da7a62ec74a7871bd4
+eff43f2f0effa96c3e83a87f4bbe5a039724b7fc

Diff:

Revision 5 (+731 -168)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
David Trowbridge
  1. 
      
    1. This should probably also get a ship-it from Christian before landing, due to its size and impact.

  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    Should use double-backticks, since this is rst and not markdown.

  3. 
      
Christian Hammond
  1. 
      
    1. This looks like a lot of comments, but the majority are "Here too" comments for simple fixes.

  2. GitLab API tests should continue to test API v3, with additional tests for API v4, rather than just moving them all to v4. This ensures we don't regress for users. Might be worth making a test class for each.

  3. Make sure you build the docs and check for new errors. There are lots of syntax and reference errors throughout that would come up.

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

    This should probably be a HostingServiceError.

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

    You should be able to just pass self.causes into the %r directly. It'll repr the list, which will then auto-repr each item in the list.

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

    Help text shouldn't use <br>. One long string is better here.

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

    , unused after the type.

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

    Other docs don't do "Raised if..." That's implied and can be left off.

    For the last one, "occurred" would be more correct.

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

    "SHA1", and of the blob for the file.

  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
  11. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     

    , unused

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

    Same comment about the "Raised if ..."

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

    Same notes as above.

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

    Same notes as above about "Raised if ..."

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

    While you're here, this should be "Return a list of branches."

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

    This isn't correct.

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

    Same notes as above.

  18. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    This isn't correct. Shoud be reviewboard.scmtools.core.Commit.

  19. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     

    Same notes as above with "Raised if ..."

  20. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    reviewboard.scmtools.core.Commit

  21. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     

    Same notes as above with "Raised if ..."

  22. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    Can you include the comment from above here as well, about why we're checking both message and title?

  23. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     
     
     
     

    Same notes about "Raised if ..."

  24. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    I assume it's checking both for GitLab API version differences? Can you document those here?

  25. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     

    While here, can you add a blank line between these?

  26. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     
     

    Same notes about "Raised if ..."

  27. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     
     

    Same here.

  28. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     
     

    Same here.

  29. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
  30. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     

    Will the old version work on the new API?

  31. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    Typo: "reviewboard"

  32. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     

    Same note about "Raised if ..."

  33. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     

    Here too.

  34. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     
     

    Same notes about "Raised if ..."

  35. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     

    This is a pretty weird and hard-to-read statement, particularly with all the layers of nots and the ^. Given that it's an internal function, and we're responsible for the arguments, I'm not sure we need an exception (which is also not helpful to users if we do screw up with the call and it bubbles up). It's also probably not a big deal if all three are passed in, in practice. How about just doing this further down:

    if url:
        assert not hosting_url
        assert not path
    else:
        url = ...
    
  36. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     
     

    Same comments about "Raised if ..."

  37. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    "Return the version ..."

  38. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    Syntax error. Missing the backtick.

  39. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    I don't see how we'd ever be in a case where the or would occur. If we hit an exception, we won't get a value here. If we get a result at all, it's going to have a value, due to the implementation in _try_api_versions. I want to make sure I'm not missing something.

  40. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     

    Typo: "arguments"

  41. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     
     
     
     
     
     

    Same notes about the "Raised if ..."

  42. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     

    This should never happen. If it does, it's an internal failure. We should assert.

  43. You can use basestring.

  44. You can use basestring.

  45. I might be wrong about some part of this, but according urlopen docs, the first argument to urlopen is url, not a request.

    Naming of parameters should also be consistent now, due to more strict kgb internals.

    Same with all other fake _urlopen functions.

  46. assertIsNone

  47. 
      
Loading...