david got a fish trophy!

Summary

Add support for getting diffs from GitLab APIv4.

Review Request #12221 — Created March 31, 2022 and submitted April 5, 2022, 11:28 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

f00f1f6...

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done


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.

Issues

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	April 1, 2022, 4:23 p.m.
Can you add unit tests for some combinations of the headers we're looking for? Make sure we don't break if …	chipx86	April 4, 2022, 10:43 a.m.
Can you verify if this works on both Python 2 and 3?	chipx86	April 4, 2022, 10:44 a.m.
E501 line too long (80 > 79 characters)	reviewbot	March 31, 2022, 2:08 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 31, 2022, 2:09 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 31, 2022, 2:09 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 31, 2022, 2:09 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 31, 2022, 2:09 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 31, 2022, 2:09 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 31, 2022, 2:09 p.m.
Small nit: "URL"	chipx86	April 1, 2022, 4:06 p.m.
Rather than searching each of these per file, maybe combine into one regex, so we only need to search once?	chipx86	April 4, 2022, 10:31 a.m.
I don't think - needs to be escaped.	chipx86	April 4, 2022, 10:31 a.m.
Too many blank lines.	chipx86	April 1, 2022, 4:06 p.m.
E501 line too long (82 > 79 characters)	reviewbot	April 1, 2022, 4:28 p.m.
The index line may also contain a UNIX file mode at the end, so we may want to just avoid …	chipx86	April 4, 2022, 4:14 p.m.
F841 local variable 'diff_rsp' is assigned to but never used	reviewbot	April 4, 2022, 10:45 a.m.
F841 local variable 'commit' is assigned to but never used	reviewbot	April 4, 2022, 10:45 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/hostingsvcs/tests/test_gitlab.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gitlab.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gitlab.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gitlab.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gitlab.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gitlab.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```

Commit:

b821d818906987b337238d33ebad04a2d52c30cc

a8a0ef55e90cc0b35132fd99dc371af5b832a19c

Diff:

Revision 2 (+458 -152)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/gitlab.py
	reviewboard/hostingsvcs/testing/testcases.py
	reviewboard/hostingsvcs/tests/test_gitlab.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you verify if this works on both Python 2 and 3?

david April 1, 2022, 4:08 p.m.

So far this is only implemented/tested on 3.0.x, so Python 2 only. I'll do a test port to 4.0.x once other issues are resolved.

reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Small nit: "URL"
```
reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Too many blank lines.
```

The issue has been resolved. Show all issues

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 the git --diff, we won't add a redundant one.

Commit:

a8a0ef55e90cc0b35132fd99dc371af5b832a19c

06befa01b3901172a63df5fdafe90b2015d0a3fd

Diff:

Revision 3 (+478 -152)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/gitlab.py
	reviewboard/hostingsvcs/testing/testcases.py
	reviewboard/hostingsvcs/tests/test_gitlab.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
The issue has been dropped. Show all issues
```
E501 line too long (82 > 79 characters)
```

Commit:

06befa01b3901172a63df5fdafe90b2015d0a3fd

da54956364bbf5d9dd3f4cef5a57f621224935a6

Diff:

Revision 4 (+479 -152)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/gitlab.py
	reviewboard/hostingsvcs/testing/testcases.py
	reviewboard/hostingsvcs/tests/test_gitlab.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.


sddsfhjk sdfhjksdfh jksfhk jsdhf kasjldhf jklasdhfkjsdahf jksadhfkj asdfjh sadklfas



def abc

chipx86 April 2, 2022, 12:25 p.m.
```
And I stand by every word of it.
```
david April 4, 2022, 10:32 a.m.

The issue has been resolved. Show all issues

Can you add unit tests for some combinations of the headers we're looking for? Make sure we don't break if they add some or all of those?

reviewboard/hostingsvcs/gitlab.py (Diff revisions 2 - 4)

The issue has been resolved. Show all issues

Rather than searching each of these per file, maybe combine into one regex, so we only need to search once?

reviewboard/hostingsvcs/gitlab.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
I don't think - needs to be escaped.
```
1. david April 4, 2022, 10:32 a.m.
  Changed to just look for the +++ line since if we have one we can expect both.

Commit:

da54956364bbf5d9dd3f4cef5a57f621224935a6

e762ddb00075b8bed5c73ff4106e9a593c0b90b9

Diff:

Revision 5 (+766 -152)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/gitlab.py
	reviewboard/hostingsvcs/testing/testcases.py
	reviewboard/hostingsvcs/tests/test_gitlab.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/tests/test_gitlab.py (Diff revision 5)
The issue has been dropped. Show all issues
```
F841 local variable 'diff_rsp' is assigned to but never used
```
reviewboard/hostingsvcs/tests/test_gitlab.py (Diff revision 5)
The issue has been dropped. Show all issues
```
F841 local variable 'commit' is assigned to but never used
```

Commit:

e762ddb00075b8bed5c73ff4106e9a593c0b90b9

e6c141d530d48b603a1ab82397a9b3a3be5a294b

Diff:

Revision 6 (+775 -152)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/gitlab.py
	reviewboard/hostingsvcs/testing/testcases.py
	reviewboard/hostingsvcs/tests/test_gitlab.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

The index line may also contain a UNIX file mode at the end, so we may want to just avoid the $.

For example:

index 3f786850e387550fdab836ed7e6dc881de23001b..89e6c98d92887913cadf06b2adb97f26cde4849b 100644

Commit:

e6c141d530d48b603a1ab82397a9b3a3be5a294b

f00f1f6557c01867f220fb04982b2ee145dc1f35

Diff:

Revision 7 (+775 -152)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/gitlab.py
	reviewboard/hostingsvcs/testing/testcases.py
	reviewboard/hostingsvcs/tests/test_gitlab.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (3298f5f)