Summary

Add support to provide review_request_id as URL

Review Request #11107 — Created July 31, 2020 and submitted Aug. 17, 2020, 5:13 p.m.

Information

Owner

misery

Repository

RBTools

Branch

release-1.0.x

Bugs

Depends On

Commit

57db8f9...

Reviewers

Groups

rbtools

People

Description

If a developer (or CI) gets a review request to review he
will get a whole URL because it was copy+pasted from browser.
This change allows to easily use that URL to get the patch
of given review request.

Testing Done

Provided an URL to rbt patch and saw that it extract the
review_request_id and the diff_id correctly and saw that
it correctly used the host.

Also provided an URL as interdiff format /4-5/ and
saw it was refused.

Issues

Description	From	Last Updated
How about making a reusable function in rbtools/utils/review_request.py (parse_review_request_url())?	chipx86	Aug. 3, 2020, 10:43 a.m.
This should be utilizing the six module's compatibility imports. At the top of the file, in the correct import group …	chipx86	Aug. 3, 2020, 10:44 a.m.
I've been mulling this over. I think rather than make an assumption about the last bit of a URL containing …	chipx86	Aug. 3, 2020, 10:47 a.m.
E302 expected 2 blank lines, found 1	reviewbot	Aug. 3, 2020, 10:45 a.m.
F821 undefined name 'm'	reviewbot	Aug. 3, 2020, 10:44 a.m.
F821 undefined name 'm'	reviewbot	Aug. 3, 2020, 10:44 a.m.
F821 undefined name 'm'	reviewbot	Aug. 3, 2020, 10:44 a.m.
W391 blank line at end of file	reviewbot	Aug. 3, 2020, 10:44 a.m.

flake8 passed.

JSHint passed.

This is a good idea, and I could see it being useful outside of rbt patch (rbt land would benefit from it).

I have some suggestions for enhancing the way this works. Let me know if you want us to take it on.

The issue has been resolved. Show all issues

How about making a reusable function in rbtools/utils/review_request.py (parse_review_request_url())?

rbtools/commands/patch.py (Diff revision 1)

The issue has been dropped. Show all issues

This should be utilizing the six module's compatibility imports. At the top of the file, in the correct import group (and organized alphabetically), you'd want:

from django.utils.six.moves.urllib.parse import urlparse

rbtools/commands/patch.py (Diff revision 1)

The issue has been resolved. Show all issues

I've been mulling this over. I think rather than make an assumption about the last bit of a URL containing an ID, the aforementioned function for this should pattern match using a regex.
That would look something like:

re.compile(r'^(?<server_url>https?://.*/(?:/s/[^/]+/)?)r/(?<review_request_id>\d+)/?')


This would happen URLs with/without a subdomain, with/without a Local Site, and would capture even if passing in a diff URL.
This could potentially be extended to even look for a diff/(\d+)/, working as an alternative to providing --diff-revision. Though that's certainly not something that needs to be implemented in this change.

misery Aug. 3, 2020, 10:46 a.m.

Thanks, that was a good idea. I switched to your regex and added diff_id as an optional attribute.
Feel free to adopt this change for rbt land. :-)

Change Summary:

Switched to regex and added diff_id as group

Commit:

fb063a852381260110f0a5ec7f6d317416b95bca

4cbb3d98e960d85169ce5c8e287bbe3c4f995d75

Diff:

Revision 2 (+29 -1)

Show changes

	rbtools/commands/patch.py
	rbtools/utils/review_request.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/utils/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E302 expected 2 blank lines, found 1
```
rbtools/utils/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'm'
```
rbtools/utils/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'm'
```
rbtools/utils/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'm'
```
rbtools/utils/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```

Change Summary:

flake8

Commit:

4cbb3d98e960d85169ce5c8e287bbe3c4f995d75

52b8a21dac60c4de8c7ffb0f92465dcf39028623

Diff:

Revision 3 (+29 -1)

Show changes

	rbtools/commands/patch.py
	rbtools/utils/review_request.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Testing Done:: Provided an URL to rbt patch and saw that it extract the
~ review_request_id correctly and saw that it correctly
~ used the netloc from URL.
~ review_request_id and the diff_id correctly and saw that
~ it correctly used the host.
+
+
Also provided an URL as interdiff format /4-5/ and
+ saw it was refused.

Change Summary:

Allow --diff-revision if URL is provided.

Commit:

52b8a21dac60c4de8c7ffb0f92465dcf39028623

57db8f904c611be96ad8990cdb9b144b0b156a0e

Diff:

Revision 4 (+30 -1)

Show changes

	rbtools/commands/patch.py
	rbtools/utils/review_request.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

Going to make some small changes to docstrings and add support for rbt land, then get this in. Thanks!

Status:: Completed
Change Summary:: Pushed to release-2.0.x (66ae00a)

		Provided an URL to rbt patch and saw that it extract the
~		review_request_id correctly and saw that it correctly
~		used the netloc from URL.
	~	review_request_id and the diff_id correctly and saw that
	~	it correctly used the host.
	+
	+	Also provided an URL as interdiff format `/4-5/` and
	+	saw it was refused.