Add fetching of git/hg repositories

Review Request #9502 — Created Jan. 20, 2018 and submitted

Information

ReviewBot
release-1.0.x
d42c494...

Reviewers

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.

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.

Description From Last Updated

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

daviddavid

W503 line break before binary operator

reviewbotreviewbot

TODO: needs to paginate here

miserymisery

Can we call this review_board_servers?

daviddavid

Should be "Review Board"

daviddavid

Same here.

daviddavid

Let's say: If you have many workers and repositories, it may not be feasible to configure repositories by hand. You …

daviddavid

How about "Automatically Fetch Repositories From Review Board"

daviddavid

useable -> usable

daviddavid

reviewboard -> Review Board server.

daviddavid

Let's rewrite this to say: The repository path or mirror_path field must be the URL of a repository which is …

daviddavid

Please add an "Args" section to this docstring.

daviddavid

This should say tasks.py rather than celery.py

daviddavid

Indent this one more space.

daviddavid

Maybe switch the success/error cases here?

daviddavid

Typo: useable -> usable

daviddavid

Please add an "Args" section to this docstring.

daviddavid

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

daviddavid

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

daviddavid

Can we call this fetch_repositories?

brenniebrennie

url is missing.

brenniebrennie

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

brenniebrennie

Same here re: tuples vs lists.

brenniebrennie

There might be ssh:// URLs too? If it isn't a file path, should we parse path and check (a) it …

brenniebrennie

No blank line here.

brenniebrennie

HTTP Technically, it could also be exposed over SSH, no?

brenniebrennie
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

misery
misery
misery
misery
  1. 
      
  2. bot/reviewbot/repositories.py (Diff revision 3)
     
     
     

    TODO: needs to paginate here

    1. how to do that here??

    2. 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:
          # ...
      
    3. Thank you very much!

  3. 
      
misery
misery
  1. 
      
  2. Ping! Any chance to merge it?

    1. :-( contribution to review board is no fun

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

  3. 
      
david
  1. 
      
  2. bot/reviewbot/config.py (Diff revision 4)
     
     

    Can we call this review_board_servers?

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

    Should be "Review Board"

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

    Same here.

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

    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.

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

    How about "Automatically Fetch Repositories From Review Board"

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

    useable -> usable

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

    reviewboard -> Review Board server.

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

    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"

  10. 
      
misery
david
  1. 
      
  2. In your description and testing done, "ReviewBot" -> "Review Bot"

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

    Please add an "Args" section to this docstring.

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

    This should say tasks.py rather than celery.py

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

    Indent this one more space.

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

    Maybe switch the success/error cases here?

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

    Typo: useable -> usable

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

    Please add an "Args" section to this docstring.

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

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

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

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

  11. 
      
misery
brennie
  1. 
      
  2. bot/reviewbot/repositories.py (Diff revision 6)
     
     

    Can we call this fetch_repositories?

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

    url is missing.

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

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

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

    Same here re: tuples vs lists.

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

    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') ?

    1. 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.

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

    No blank line here.

  8. docs/reviewbot/configuration.rst (Diff revision 6)
     
     

    HTTP

    Technically, it could also be exposed over SSH, no?

    1. Fixed.... ssh: see above

  9. 
      
misery
misery
misery
  1. 
      
  2. Happy Birthday, Review Request! :(

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

  2. 
      
misery
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (67d4f05)
Loading...