Update and streamline remote repository detection.

Review Request #11536 — Created March 21, 2021 and submitted

david
RBTools
master
rbtools

This is a major rework of the remote repository detection mechanism
within RBTools. The old implementation had a lot of issues, worst of
which was that there was a lot of inconsistency across different tool
implementations (for example, rbt post had a lot of additional
detection code that wasn't shared with other tools).

In the new implementation, we first try to fetch remote repositories,
filtering by the configured repository name, or detected repository
path(s). If this returns a single result, we can immediately return
that. If that fails, it means we're in a situation where the configured
paths on the server are different from the local path (including any
mirror_path), and we need to match in a different way. Individual tools
may implement find_matching_server_repository in order to do
tool-specific comparisons (which right now is done by SVN with
repository UUID and ClearCase with VOB UUID). This new method replaces
the old RepositoryInfo.find_server_repository_info method, which
always felt super ugly and clunky. In lieu of that, RepositoryInfo
subclasses can implement update_from_remote, which allows them to
include information from the remote repository or repository info
resource.

Commands which want to make use of the repository can now set the
needs_repository attribute. In this case, the command initialization
will handle finding the remote repository and updating the
RepositoryInfo appropriately. This corrects a couple issues where
commands were using the repository but forgetting to call the old
find_server_repository_info, potentially ending up with incorrect
data. The setup-repo command works a little differently because it
needs to potentially do that multiple times, but it uses the same
underlying mechanism to attempt to pre-filter the list of matching
repositories in a useful way.

This includes several other improvements and cleanups along the way:

  • Various places that fetch repositories to find matches have all been
    updated to use the locally detected tool to filter the response from
    the server. In the case where the server has a mix of repository
    types, this will dramatically improve the results.
  • rbt setup-repo's server initialization has been fixed to merge
    handling of --server and interactive entry. This both improves the
    code, and has the side effect of making it so that if the server
    specified on the command line doesn't work, it will prompt the user.
  • Ran unit tests.
  • Verified that repository match fetches properly passed tool= for API
    requests to filter the responses based on the locally detected tool.
  • rbt land: Tested basic behavior.
  • rbt patch: Tested basic behavior.
  • rbt post: Posted new and updated changes against Git, including with
    -u. Created a remote SVN repository and posted purely remote commits
    using --repository-url.
  • rbt setup-repo: Tested with both --server and specifying the
    server name interactively.
  • rbt stamp: Tested basic behavior.
  • rbt status: tested both with and without --all. Verified that
    repository detection was making the most minimal API requests possible
    based on various .reviewboardrc configurations.
Summary
Update and streamline remote repository detection.
Description From Last Updated

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

F841 local variable 'e' is assigned to but never used

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (166 > 79 characters)

reviewbotreviewbot

E501 line too long (170 > 79 characters)

reviewbotreviewbot

E501 line too long (161 > 79 characters)

reviewbotreviewbot

E501 line too long (157 > 79 characters)

reviewbotreviewbot

E501 line too long (127 > 79 characters)

reviewbotreviewbot

E501 line too long (118 > 79 characters)

reviewbotreviewbot

E501 line too long (145 > 79 characters)

reviewbotreviewbot

E501 line too long (146 > 79 characters)

reviewbotreviewbot

E501 line too long (153 > 79 characters)

reviewbotreviewbot

E501 line too long (146 > 79 characters)

reviewbotreviewbot

E501 line too long (111 > 79 characters)

reviewbotreviewbot

Can you add a Version Added and Type (in that order)?

chipx86chipx86

Can we make this a list of strings instead? I feel like this would be a bit nicer.

chipx86chipx86

Can you add Version Added?

chipx86chipx86

Can you add Version Added?

chipx86chipx86

Can you add Deprecated in here?

chipx86chipx86

Can we move these private methods after the public ones?

chipx86chipx86

This needs to be a full class path.

chipx86chipx86

No blank line here.

chipx86chipx86

This also needs to be a full class path.

chipx86chipx86

Can you add Deprecated here, and any other methods that emit a deprecation warning?

chipx86chipx86

The pattern I've been moving to is to import kgb directly, so we don't have to bother updating the import …

chipx86chipx86

E501 line too long (166 > 79 characters)

reviewbotreviewbot

Review Bot is going to complain about this forever. We could maybe do: ('http://...... '......'): { .... }, ... Also, …

chipx86chipx86

E501 line too long (170 > 79 characters)

reviewbotreviewbot

E501 line too long (161 > 79 characters)

reviewbotreviewbot

E501 line too long (157 > 79 characters)

reviewbotreviewbot

E501 line too long (127 > 79 characters)

reviewbotreviewbot

E501 line too long (118 > 79 characters)

reviewbotreviewbot

While here, want to move to @self.spy_for(urlopen)?

chipx86chipx86

Can we move this to the top? Or is this leftover debugging?

chipx86chipx86

Can you add Version Added and Type (in that order)?

chipx86chipx86

Can these be keyword arguments?

chipx86chipx86

The description shouldn't be indented.

chipx86chipx86

Can you add a Version Added?

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Alternatively: query.pop('path', None)

chipx86chipx86

To render correctly, this needs to be above Args, so it fits with the doc body.

chipx86chipx86

I like the , deprecated. We haven't done that before, but I think that's a great tag. We should put …

chipx86chipx86

A bunch of args (these onward) are documented as non-optional, but have a preset value. I assume this is because …

chipx86chipx86

This will also need to move above Args.

chipx86chipx86

For new code, let's do import kgb and then reference kgb.SpyAgency. This makes it easy to later use kgb.SpyOpReturn or …

chipx86chipx86

No need for parens here, or in the ones below.

chipx86chipx86

E501 line too long (145 > 79 characters)

reviewbotreviewbot

E501 line too long (146 > 79 characters)

reviewbotreviewbot

E501 line too long (153 > 79 characters)

reviewbotreviewbot

E501 line too long (146 > 79 characters)

reviewbotreviewbot

E501 line too long (111 > 79 characters)

reviewbotreviewbot

Let's do: @self.spy_for(urlopen)

chipx86chipx86

Shouldn't these all say "Testing get_repository_resource"?

chipx86chipx86

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

Before "Args". "Args" and "Returns" are special, and must come after.

chipx86chipx86

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

This needs to move before Returns, since it'll be part of the description text.

chipx86chipx86

Same here.

chipx86chipx86

Can you update this to use the full class path?

chipx86chipx86

Missing space before "In".

chipx86chipx86

Looking at the latest revision, do we still have any reason to default api_client and api_root to None?

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

flake8

david
  1. 
      
  2. 
      
david
Review request changed

Commits:

Summary
-
Update and streamline remote repository detection.
+
Update and streamline remote repository detection.

Diff:

Revision 2 (+1874 -776)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

chipx86
  1. David, this is great. It really came together.

    My comments are almost entirely doc-related, with a couple small suggestions here and there. The overall architecture, what an improvement.

  2. rbtools/clients/__init__.py (Diff revision 2)
     
     

    Can you add a Version Added and Type (in that order)?

  3. rbtools/clients/__init__.py (Diff revision 2)
     
     
     

    Can we make this a list of strings instead? I feel like this would be a bit nicer.

    1. The vast majority of clients will only ever have one name in here, and the only use is for sticking into the API request. I'd rather keep it slightly uglier for the definitions (in the rare case where a single client class in RBTools can map to multiple client classes in Review Board) and avoid having to join a list when we make the API requests.

  4. rbtools/clients/__init__.py (Diff revision 2)
     
     

    Can you add Version Added?

  5. rbtools/clients/__init__.py (Diff revision 2)
     
     

    Can you add Version Added?

  6. rbtools/clients/__init__.py (Diff revision 2)
     
     

    Can you add Deprecated in here?

  7. rbtools/clients/clearcase.py (Diff revision 2)
     
     

    Can we move these private methods after the public ones?

  8. rbtools/clients/clearcase.py (Diff revision 2)
     
     

    This needs to be a full class path.

  9. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
     

    No blank line here.

  10. rbtools/clients/clearcase.py (Diff revision 2)
     
     

    This also needs to be a full class path.

  11. rbtools/clients/clearcase.py (Diff revision 2)
     
     

    Can you add Deprecated here, and any other methods that emit a deprecation warning?

  12. rbtools/clients/tests/test_svn.py (Diff revision 2)
     
     

    The pattern I've been moving to is to import kgb directly, so we don't have to bother updating the import list to use any specific kgb tooling.

  13. rbtools/clients/tests/test_svn.py (Diff revision 2)
     
     

    Review Bot is going to complain about this forever. We could maybe do:

    ('http://......
     '......'): {
    
        ....
    },
    ...
    

    Also, maybe take the base URL out of these and put them into a variable to both shorten these and make that part more contextually apparent?

  14. rbtools/clients/tests/test_svn.py (Diff revision 2)
     
     

    While here, want to move to @self.spy_for(urlopen)?

  15. rbtools/clients/tests/test_svn.py (Diff revision 2)
     
     

    Can we move this to the top? Or is this leftover debugging?

  16. rbtools/commands/__init__.py (Diff revision 2)
     
     
     
     
     

    Can you add Version Added and Type (in that order)?

  17. rbtools/commands/__init__.py (Diff revision 2)
     
     
     

    Can these be keyword arguments?

  18. rbtools/commands/stamp.py (Diff revision 2)
     
     
     
     
     

    The description shouldn't be indented.

  19. rbtools/utils/repository.py (Diff revision 2)
     
     

    Can you add a Version Added?

  20. rbtools/utils/repository.py (Diff revision 2)
     
     

    Missing a trailing comma.

  21. rbtools/utils/repository.py (Diff revision 2)
     
     
     

    Alternatively:

    query.pop('path', None)
    
  22. rbtools/utils/review_request.py (Diff revision 2)
     
     
     
     
     
     

    To render correctly, this needs to be above Args, so it fits with the doc body.

  23. rbtools/utils/review_request.py (Diff revision 2)
     
     

    I like the , deprecated. We haven't done that before, but I think that's a great tag. We should put that in Notion.

  24. rbtools/utils/review_request.py (Diff revision 2)
     
     
     
     
     
     

    A bunch of args (these onward) are documented as non-optional, but have a preset value. I assume this is because of the deprecated parameters preceding them. We should make sure to enforce any that are indeed required through checks at the start of the function.

  25. rbtools/utils/review_request.py (Diff revision 2)
     
     
     
     
     

    This will also need to move above Args.

  26. rbtools/utils/tests/test_repository.py (Diff revision 2)
     
     

    For new code, let's do import kgb and then reference kgb.SpyAgency. This makes it easy to later use kgb.SpyOpReturn or other methods without extending the import list.

  27. rbtools/utils/tests/test_repository.py (Diff revision 2)
     
     

    No need for parens here, or in the ones below.

  28. rbtools/utils/tests/test_repository.py (Diff revision 2)
     
     

    Let's do:

    @self.spy_for(urlopen)
    
  29. rbtools/utils/tests/test_repository.py (Diff revision 2)
     
     

    Shouldn't these all say "Testing get_repository_resource"?

  30. 
      
david
Review request changed

Commits:

Summary
-
Update and streamline remote repository detection.
+
Update and streamline remote repository detection.

Diff:

Revision 3 (+2358 -1112)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. 
      
david
  1. 
      
  2. 
      
david
Review request changed

Commits:

Summary
-
Update and streamline remote repository detection.
+
Update and streamline remote repository detection.

Diff:

Revision 4 (+2358 -1112)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 5)
     
     
     
     

    This needs to move before Returns, since it'll be part of the description text.

  3. rbtools/clients/__init__.py (Diff revision 5)
     
     
     
     

    Same here.

  4. rbtools/commands/stamp.py (Diff revision 5)
     
     

    Can you update this to use the full class path?

  5. rbtools/utils/repository.py (Diff revision 5)
     
     

    Missing space before "In".

  6. rbtools/utils/review_request.py (Diff revision 5)
     
     
     

    Looking at the latest revision, do we still have any reason to default api_client and api_root to None?

  7. 
      
david
  1. 
      
  2. 
      
david
chipx86
  1. 
      
  2. rbtools/clients/__init__.py (Diff revisions 4 - 6)
     
     
     

    Before "Args". "Args" and "Returns" are special, and must come after.

  3. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (3fbea5f)
Loading...