Summary

Add fetching of git/hg repositories

Review Request #9502 — Created Jan. 20, 2018 and submitted Feb. 4, 2019, 1:53 p.m.

Information

Owner

misery

Repository

ReviewBot

Branch

release-1.0.x

Bugs

Depends On

~~9498~~

Commit

d42c494...

Reviewers

Groups

reviewbot

People

Description

If there are a lot of Review Bot worker it will be

very expensive to maintain a repository configuration

manually. Since a reviewboard instance already knows

the configuration it is now be possible to configure

multiple reviewboard instances and let the Review Bot

fetch git and hg repositories automatically.
If a repository has a "path" or "mirror_path" that

can be access locally or has a scheme starting with

"http" or "git" it will be added to known repository.

Testing Done

Added some repositories in reviewboard with path
and mirror_path. Some with non-existing local path
and mirror_path with "http" scheme.

Saw that the Review Bot finds all accessible
repositories are added correctly.

Tested with and without authentication.

Issues

Description	From	Last Updated
In your description and testing done, "ReviewBot" -> "Review Bot"	david	Nov. 28, 2018, 7 a.m.
W503 line break before binary operator	reviewbot	Jan. 20, 2018, 5:20 a.m.
TODO: needs to paginate here	misery	July 9, 2018, 10:38 p.m.
Can we call this review_board_servers?	david	Sept. 27, 2018, 6:27 a.m.
Should be "Review Board"	david	Sept. 27, 2018, 6:28 a.m.
Same here.	david	Sept. 27, 2018, 6:28 a.m.
Let's say: If you have many workers and repositories, it may not be feasible to configure repositories by hand. You …	david	Sept. 27, 2018, 6:29 a.m.
How about "Automatically Fetch Repositories From Review Board"	david	Sept. 27, 2018, 6:29 a.m.
useable -> usable	david	Sept. 27, 2018, 6:29 a.m.
reviewboard -> Review Board server.	david	Sept. 27, 2018, 6:30 a.m.
Let's rewrite this to say: The repository path or mirror_path field must be the URL of a repository which is …	david	Sept. 27, 2018, 6:38 a.m.
Please add an "Args" section to this docstring.	david	Nov. 28, 2018, 6:26 a.m.
This should say tasks.py rather than celery.py	david	Nov. 28, 2018, 6:26 a.m.
Indent this one more space.	david	Nov. 28, 2018, 6:27 a.m.
Maybe switch the success/error cases here?	david	Nov. 28, 2018, 6:27 a.m.
Typo: useable -> usable	david	Nov. 28, 2018, 6:27 a.m.
Please add an "Args" section to this docstring.	david	Nov. 28, 2018, 6:33 a.m.
I'd rather keep this explicitly as "git" and not introduce the case insensitivity.	david	Nov. 28, 2018, 6:33 a.m.
Can be repo_type in ('hg', 'mercurial') (and probably should keep everything lower case)	david	Nov. 28, 2018, 6:34 a.m.
Can we call this fetch_repositories?	brennie	Nov. 28, 2018, 10:05 a.m.
url is missing.	brennie	Nov. 28, 2018, 10:06 a.m.
Instead of constructing a list, you can use a tuple here.	brennie	Nov. 28, 2018, 10:09 a.m.
Same here re: tuples vs lists.	brennie	Nov. 28, 2018, 10:09 a.m.
There might be ssh:// URLs too? If it isn't a file path, should we parse path and check (a) it …	brennie	Nov. 28, 2018, 3:17 p.m.
No blank line here.	brennie	Nov. 28, 2018, 10:10 a.m.
HTTP Technically, it could also be exposed over SSH, no?	brennie	Nov. 28, 2018, 3:17 p.m.

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/repositories.py (Diff revision 1)
Show all issues
```
W503 line break before binary operator
```

Commit:

-	63f565684065895ddcce97398813dad0af81f6b4
+	b85cd6709c9e4dcf7bed1787e603b696d5340648

Diff:

Revision 2 (+94 -11)

Show changes

	bot/reviewbot/config.py
	bot/reviewbot/repositories.py
	docs/reviewbot/reviewbot/configuration.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

rebase

Branch:

-	master
+	release-1.0.x

Commit:

-	b85cd6709c9e4dcf7bed1787e603b696d5340648
+	92568a9b7eb2830f955e80b6547c09a8db4c49d1

Diff:

Revision 3 (+94 -11)

Show changes

	bot/reviewbot/config.py
	bot/reviewbot/repositories.py
	docs/reviewbot/configuration.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

~		If there are a lof of ReviewBot worker it will be
	~	If there are a lot of ReviewBot worker it will be
		very expensive to maintain a repository configuration
		manually. Since a reviewboard instance already knows
		the configuration it is now be possible to configure
		multiple reviewboard instances and let the ReviewBot
		fetch git and hg repositories automatically.

		If a repository has a "path" or "mirror_path" that
		can be access locally or has a scheme starting with
		"http" or "git" it will be added to known repository.

bot/reviewbot/repositories.py (Diff revision 3)

Show all issues

TODO: needs to paginate here

misery July 7, 2018, 12:51 p.m.
```
how to do that here??
```

brennie July 9, 2018, 3:14 p.m.

RBTools 1.0 has support to do this built in. If you upgrade the dependency to 1.0, you can iterate with one of the follwoing:

for page in root.get_repositories(...).all_pages:
    for repository in page:
        # ...

for repository in root.get_repositories(...).all_items:
    # ...

misery July 9, 2018, 10:40 p.m.
```
Thank you very much!
```

Commit:

-	92568a9b7eb2830f955e80b6547c09a8db4c49d1
+	5b01f073dcc5d8767db22b5a0f5d24d200923028

Diff:

Revision 4 (+95 -12)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/repositories.py
	docs/reviewbot/configuration.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

1. misery Oct. 17, 2018, 12:50 p.m.
  ping?

Ping! Any chance to merge it?

misery Nov. 5, 2018, 2:37 p.m.

:-( contribution to review board is no fun

david Nov. 5, 2018, 2:43 p.m.

I'm sorry. We've been very busy these last couple months.

bot/reviewbot/config.py (Diff revision 4)
Show all issues
```
Can we call this review_board_servers?
```
bot/reviewbot/repositories.py (Diff revision 4)
Show all issues
```
Should be "Review Board"
```
bot/reviewbot/repositories.py (Diff revision 4)
Show all issues
```
Same here.
```

docs/reviewbot/configuration.rst (Diff revision 4)

Show all issues

Let's say:

If you have many workers and repositories, it may not be feasible to configure repositories by hand. You can also configure a list of Review Board servers to fetch all supported repositories from.

docs/reviewbot/configuration.rst (Diff revision 4)

Show all issues

How about "Automatically Fetch Repositories From Review Board"

docs/reviewbot/configuration.rst (Diff revision 4)
Show all issues
```
useable -> usable
```
docs/reviewbot/configuration.rst (Diff revision 4)
Show all issues
```
reviewboard -> Review Board server.
```

docs/reviewbot/configuration.rst (Diff revision 4)

Show all issues

Let's rewrite this to say:

The repository path or mirror_path field must be the URL of a repository which is accessible to the Review Bot worker. If you use a local file path for your repository and the worker is not running on the same host as the Review Board server, you must also expose the repository over http and set the mirror_path.

This should probably also be a separate paragraph from the "manually configured repositories will override"

Commit:

-	5b01f073dcc5d8767db22b5a0f5d24d200923028
+	e903411cf6b91f3b3a6636ba57928e27f32b597f

Diff:

Revision 5 (+99 -12)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/repositories.py
	docs/reviewbot/configuration.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

In your description and testing done, "ReviewBot" -> "Review Bot"

bot/reviewbot/repositories.py (Diff revision 5)
Show all issues
```
Please add an "Args" section to this docstring.
```
bot/reviewbot/repositories.py (Diff revision 5)
Show all issues
```
This should say tasks.py rather than celery.py
```
bot/reviewbot/repositories.py (Diff revision 5)
Show all issues
```
Indent this one more space.
```
bot/reviewbot/repositories.py (Diff revision 5)
Show all issues
```
Maybe switch the success/error cases here?
```
bot/reviewbot/repositories.py (Diff revision 5)
Show all issues
```
Typo: useable -> usable
```
bot/reviewbot/repositories.py (Diff revision 5)
Show all issues
```
Please add an "Args" section to this docstring.
```

bot/reviewbot/repositories.py (Diff revision 5)

Show all issues

I'd rather keep this explicitly as "git" and not introduce the case insensitivity.

bot/reviewbot/repositories.py (Diff revision 5)

Show all issues

Can be repo_type in ('hg', 'mercurial') (and probably should keep everything lower case)

Description:

~		If there are a lot of ReviewBot worker it will be
	~	If there are a lot of Review Bot worker it will be
		very expensive to maintain a repository configuration
		manually. Since a reviewboard instance already knows
		the configuration it is now be possible to configure
~		multiple reviewboard instances and let the ReviewBot
	~	multiple reviewboard instances and let the Review Bot
		fetch git and hg repositories automatically.

		If a repository has a "path" or "mirror_path" that
		can be access locally or has a scheme starting with
		"http" or "git" it will be added to known repository.

Testing Done:

		Added some repositories in reviewboard with path
		and mirror_path. Some with non-existing local path
		and mirror_path with "http" scheme.

~		Saw that the ReviewBot finds all accessible
	~	Saw that the Review Bot finds all accessible
		repositories are added correctly.

		Tested with and without authentication.

Commit:

-	e903411cf6b91f3b3a6636ba57928e27f32b597f
+	28d08f628a21ee0542ef4d0279ac4151747390ee

Diff:

Revision 6 (+118 -12)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/repositories.py
	docs/reviewbot/configuration.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

bot/reviewbot/repositories.py (Diff revision 6)
Show all issues
```
Can we call this fetch_repositories?
```
bot/reviewbot/repositories.py (Diff revision 6)
Show all issues
```
url is missing.
```

bot/reviewbot/repositories.py (Diff revision 6)

Show all issues

Instead of constructing a list, you can use a tuple here.

bot/reviewbot/repositories.py (Diff revision 6)
Show all issues
```
Same here re: tuples vs lists.
```

bot/reviewbot/repositories.py (Diff revision 6)

Show all issues

There might be ssh:// URLs too?
If it isn't a file path, should we parse path and check (a) it is a valid URL and (b) its scheme is in ('http', 'https', 'ssh', 'git', 'git+ssh') ?

misery Nov. 28, 2018, 10:19 a.m.

Yeah, it is possible. But that is another feature that should be tested well because of authentication with key/password. I prefer to create a following review request to support ssh style.

bot/reviewbot/repositories.py (Diff revision 6)
Show all issues
```
No blank line here.
```
docs/reviewbot/configuration.rst (Diff revision 6)
Show all issues
```
HTTP
Technically, it could also be exposed over SSH, no?
```
1. misery Nov. 28, 2018, 10:19 a.m.
  Fixed.... ssh: see above

Commit:

-	28d08f628a21ee0542ef4d0279ac4151747390ee
+	8a0700ecc04219df1dfc99a8d1b9edbee8885ffd

Diff:

Revision 7 (+120 -12)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/repositories.py
	docs/reviewbot/configuration.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	8a0700ecc04219df1dfc99a8d1b9edbee8885ffd
+	d42c494cad5aad33d6b63f1fc9501b63a51f6053

Diff:

Revision 8 (+119 -12)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/repositories.py
	docs/reviewbot/configuration.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Happy Birthday, Review Request! :(
```

Ship it!

Making some small changes to the docs and landing. Sorry for the delay on this.

Status: Closed (submitted)

Change Summary:

Pushed to master (67d4f05)