Add support for GitLab API v4

Review Request #9383 — Created Dec. 13, 2017 and submitted

david
Review Board
release-2.5.x
a44bb26...
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...

  • 0
  • 0
  • 114
  • 2
  • 116
Description From Last Updated
david
  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. 
      
brennie
david
  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. 
      
brennie
david
  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. 
      
brennie
david
  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. 
      
brennie
david
  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. 
      
chipx86
  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. 
      
david
david
chipx86
  1. 
      
  2. Can we keep the v3 API unit tests, and just add new v4 tests, instead of replacing them?

    1. I already split the tests where it made sense but the vast majority of them are identical save for the computation of the base API path.

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

    This should probably he a HostingServiceError. That way it's properly displayed if it bubbles up.

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

    Mind putting those in alphabetical order?

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

    There's two places where we do this, and have to maintain version compatibility. How about defining a _parse_commit() method that handles this?

    I know the other location that does this has a different flow, but I have a thought there. See my next comment.

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

    And here we are.

    This method delays building the Commit until the very end, after we've dealt with the initial data and then generated the diff content. In the case where it's cached, we already have a Commit object that we throw away, though. So my thought is this that we can do this:

    if commit is None:
        ...
    
        commit_rsp = self._api_get(...)
        commit = self._parse_commit(commit_rsp)
    
    
    # Diff generation here...
    
    commit.diff = diff
    
    return commit
    

    Should simplify much of this code.

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

    Blank line between these.

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

    Alphabetical order.

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

    Do we have pagination for groups in the new API

    1. So v4 does seem to have much better pagination support, but it turns out we don't need it at all in this case--there's a new API that lets us find the repository in a single API call.

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

    Same question about repositories.

  11. Should probably be bytes

  12. 
      
david
Review request changed

Commit:

-203ff4ed40d289eedbc1e7f9d4cb597de13fadac
+4fa838eaf11479a686a607b86098dc4d996a8505

Diff:

Revision 7 (+956 -205)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/gitlab.py (Diff revisions 6 - 8)
     
     

    Private method, so this should go after all public methods.

  3. reviewboard/hostingsvcs/gitlab.py (Diff revisions 6 - 8)
     
     
     
     
     
     

    There's some old code still here.

  4. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (ab12a7e)
Loading...