david got a fish trophy!
Add support for getting diffs from GitLab APIv4.
Review Request #12221 — Created March 31, 2022 and submitted
GitLab has been slowly decommissioning old API endpoints, especially on
the SaaS gitlab.org. They recently got rid of the old diff endpoint,
which we were still using because the v4 API doesn't have a great way to
get the diffs. Unfortunately, this means we need to actually implement
support for v4.This works somewhat similarly to how the GitHub diff API works, where
instead of just getting a full copy of the diff, we get a JSON blob that
includes diff fragments for each file. This is somewhat complicated by
the fact that said diff fragments do not contain the blob SHAs for the
old and new revisions. We have to go get that information from the file
API, which is a bit chatty but at least we can just use a HEAD request
to get only the metadata instead of having to fetch the file content.
- Added a bunch of GitLab repositories and created post-commit review
requests for commits that included modified, added, and deleted files.
Saw things work every time. - Ran unit tests.
Description | From | Last Updated |
---|---|---|
Actually, one big change I'd like to have here. In the event that GitLab maybe fixes their diff, we would … |
chipx86 | |
Can you add unit tests for some combinations of the headers we're looking for? Make sure we don't break if … |
chipx86 | |
Can you verify if this works on both Python 2 and 3? |
chipx86 | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
Small nit: "URL" |
chipx86 | |
Rather than searching each of these per file, maybe combine into one regex, so we only need to search once? |
chipx86 | |
I don't think - needs to be escaped. |
chipx86 | |
Too many blank lines. |
chipx86 | |
E501 line too long (82 > 79 characters) |
reviewbot | |
The index line may also contain a UNIX file mode at the end, so we may want to just avoid … |
chipx86 | |
F841 local variable 'diff_rsp' is assigned to but never used |
reviewbot | |
F841 local variable 'commit' is assigned to but never used |
reviewbot |
- Commit:
-
b821d818906987b337238d33ebad04a2d52c30cca8a0ef55e90cc0b35132fd99dc371af5b832a19c
Checks run (2 succeeded)
-
-
Actually, one big change I'd like to have here.
In the event that GitLab maybe fixes their diff, we would end up re-corrupting it. So I think we should have logic like:
if not diff.startswith('git --diff'): # Add the git --diff if not diff.startswith('---'): # Fetch and build the file headers...
That way, if they end up adding the
---
/+++
lines, we should be able to skip the API calls. And if they add thegit --diff
, we won't add a redundant one.
- Commit:
-
06befa01b3901172a63df5fdafe90b2015d0a3fdda54956364bbf5d9dd3f4cef5a57f621224935a6
Checks run (2 succeeded)
- Commit:
-
e762ddb00075b8bed5c73ff4106e9a593c0b90b9e6c141d530d48b603a1ab82397a9b3a3be5a294b