Update and streamline remote repository detection.

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

Information

RBTools
master

Reviewers

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 ID
Update and streamline remote repository detection.
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. Testing Done: - 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. Reviewed at https://reviews.reviewboard.org/r/11536/
c107677bf3773742a6323bc59265975a03a0e1ac
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 ID
Update and streamline remote repository detection.
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. Testing Done: - 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. Reviewed at https://reviews.reviewboard.org/r/11536/
e58825a8dd743ed366fb8abf405867f323fa2c0f
Update and streamline remote repository detection.
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. Testing Done: - 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. Reviewed at https://reviews.reviewboard.org/r/11536/
16e8dd25d59e9b914277b8144d6f486719376193

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)
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    Can you add Version Added?

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

    Can you add Version Added?

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

    Can you add Deprecated in here?

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

    Can we move these private methods after the public ones?

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

    This needs to be a full class path.

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

    No blank line here.

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

    This also needs to be a full class path.

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

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

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

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

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

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

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

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

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

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

    Can these be keyword arguments?

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

    The description shouldn't be indented.

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

    Can you add a Version Added?

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

    Missing a trailing comma.

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

    Alternatively:

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

    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)
     
     
    Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
    Show all issues

    This will also need to move above Args.

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

    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)
     
     
    Show all issues

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

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

    Let's do:

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

    Shouldn't these all say "Testing get_repository_resource"?

  30. 
      
david
Review request changed
Commits:
Summary ID
Update and streamline remote repository detection.
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. Testing Done: - 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. Reviewed at https://reviews.reviewboard.org/r/11536/
16e8dd25d59e9b914277b8144d6f486719376193
Update and streamline remote repository detection.
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. Testing Done: - 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. Reviewed at https://reviews.reviewboard.org/r/11536/
861dfeb24631c849a64c754eab8f9682e358c432

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. 
      
david
  1. 
      
  2. 
      
david
Review request changed
Commits:
Summary ID
Update and streamline remote repository detection.
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. Testing Done: - 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. Reviewed at https://reviews.reviewboard.org/r/11536/
861dfeb24631c849a64c754eab8f9682e358c432
Update and streamline remote repository detection.
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. Testing Done: - 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. Reviewed at https://reviews.reviewboard.org/r/11536/
2ddd45089f35337a6239e7a041747cb5800e46c5

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

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

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

    Same here.

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

    Can you update this to use the full class path?

  5. rbtools/utils/repository.py (Diff revision 5)
     
     
    Show all issues

    Missing space before "In".

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

    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)
     
     
     
    Show all issues

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

  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (3fbea5f)