Refactor SCM tool registration using a registry

Review Request #9724 — Created March 4, 2018 and updated

jtrang
Review Board
master
dfcb8db...
reviewboard, students

Instead of registering SCM tools in the database, populate a registry
at startup. This allows for more flexibility for users to register and
unregister external tools.

Things done:
- Rename keys in registry to match the names in SCM tools classes
- Created service.py file with API for registering/unregistering tools
- Created test_registration.py to test service.py
- Renamed Tool model to LegacyTool
- Created new class Tool which is not a model. Instantiated by passing
in an SCM tool name (string). This class has the same properties as
LegacyTool but gets the SCM tool class from the registry.
- Renamed tool field in Repository model to _legacy_tool and add a
scmtool_name field. Created an evolution for this.
- Created tool property in Repository which does a lazy migration
away from LegacyTool by checking the value of scmtool_name.
- Replace usages of the legacy tool to use scmtool_name
E.g. tool = LegacyTool.objects.get(name='Subversion') to
scmtool_name='Subversion'
- Refactor RepositoryForm to use scmtool_name
- Other misc refactoring to pass tests in scmtools, hostingsvcs,
webapi, reviews, and diffviewer
- Add migratescmtools.py command.
- Add SCMToolHook extension and unit tests

Currently passing all tests inscmtools, hostingsvcs, webapi,
reviews, and diffviewer.

Tested migratescmtools.py:
- Add local repos at /admin/db/scmtools/repository/add/
- Since the RepositoryForm uses scmtool_name now, manually
update repos in Django shell to be "pre-migration"
- Check that repo scmtool_name and _legacy_tool fields are as
expected post-migration

Unit tests for SCMToolHook extension.

  • 0
  • 0
  • 24
  • 8
  • 32
Description From Last Updated
Review request changed

Description:

   

Instead of registering SCM tools in the database, populate a registry

    at startup. This allows for more flexibility for users to register and
    unregister external tools.

   
   

Things done:

    - Rename keys in registry to match the names in SCM tools classes
    - Created service.py file with API for registering/unregistering tools
    - Created test_registration.py to test service.py
    - Renamed Tool model to LegacyTool
    - Created new class Tool which is not a model. Instantiated by passing
    in an SCM tool name (string). This class has the same properties as
    LegacyTool but gets the SCM tool class from the registry.
    - Renamed tool field in Repository model to _legacy_tool and add a
    scmtool_name field. Created an evolution for this.
    - Created tool property in Repository which does a lazy migration
    away from LegacyTool by checking the value of scmtool_name.
    - Replace usages of the legacy tool to use scmtool_name
    E.g. tool = LegacyTool.objects.get(name='Subversion') to
    scmtool_name='Subversion'
    - Refactor RepositoryForm to use scmtool_name
    - Other misc refactoring to pass tests in scmtools, hostingsvcs,
~   webapi, reviews, and diffviewer

  ~ webapi, reviews, and diffviewer
  + - [WIP] add migratescmtools.py command

Testing Done:

   

Currently passing all tests inscmtools, hostingsvcs, webapi,

    reviews, and diffviewer.

   
   

Things NOT tested so far:

~   - Any tests requiring SSH/tunnels have been skipped

  ~ - Any tests requiring SSH/tunnels have been skipped
  + - No testing done for migratescmtools.py

Commit:

-3ddc389d1ab4c0ae292f3b5eacc55fe5fdcce8b9
+cce2968cdb667f4ad77694bb18fd009728e8c8f1

Diff:

Revision 3 (+3090 -2804)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

  1. 
      
  2. docs/manual/fixtures/initial_data.json (Diff revision 5)
     
     

    NOTE: For some reason, the diff captured changes in whitespace which is why there are so many line chaneges in the diff. I only changed the following lines:

    "model": "scmtools.legacytool"

    and

    "scmtool_name": "Test",

  3. 
      
  1. Mostly just questions.

  2. reviewboard/scmtools/models.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     

    Just curious, why are we using lambda's here?

    1. Ah, I should add a comment from LegacyTool here: "Templates can't access variables on a class properly. It'll attempt to instantiate the class, which will fail without the necessary parameters. So, we use the lambdas as convenient wrappers to do what the template can't do." The templates are supports_raw_file_urls, supports_ticket_auth, etc, which are set in each SCM tool class.

  3. reviewboard/scmtools/models.py (Diff revision 5)
     
     
     
     
     

    Why don't you use a lambda to assign this like the others?

    IE: Move this with the other instance variables and have the "get_scmtool_class" return the assignment?

    1. This is related to the comment above. The lambdas are wrappers, but here that is unnecessary.

  4. reviewboard/scmtools/models.py (Diff revision 5)
     
     

    Any specific reason to use " over '?

    1. Thanks! Good catch.

  5. 
      
  1. The code is generally detailed and easy to understand; but it will be nice to make sure that there should be a blank line after the last """ and the rest of the module/class body.

  2. reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
     
     

    There should be one empty line after the doc string.

  3. reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
     
     

    There should be one empty line after the doc string.

  4. reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
     
     

    We need a whitespace between the doc String and the code

  5. reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
     
     

    There should be one empty line after the doc string.

  6. reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4)
     
     

    only one empty line is needed after yield

  7. Since this is a big project the class "command" may be a little ambiguious

    1. That's a good point. I'm copying the pattern for other management commands but will double check if the name should be more specific.

    2. Confirmed that the name "Command" is required.

  8. There should be one empty line after the doc string.

  9. 
      
  1. 
      
  2. reviewboard/scmtools/service.py (Diff revision 5)
     
     

    Is there reason to keep this import here and commented out instead of just removing it?

  3. Is this commented line of code here for a purpose?

  4. 
      
Review request changed

Description:

   

Instead of registering SCM tools in the database, populate a registry

    at startup. This allows for more flexibility for users to register and
    unregister external tools.

   
   

Things done:

    - Rename keys in registry to match the names in SCM tools classes
    - Created service.py file with API for registering/unregistering tools
    - Created test_registration.py to test service.py
    - Renamed Tool model to LegacyTool
    - Created new class Tool which is not a model. Instantiated by passing
    in an SCM tool name (string). This class has the same properties as
    LegacyTool but gets the SCM tool class from the registry.
    - Renamed tool field in Repository model to _legacy_tool and add a
    scmtool_name field. Created an evolution for this.
    - Created tool property in Repository which does a lazy migration
    away from LegacyTool by checking the value of scmtool_name.
    - Replace usages of the legacy tool to use scmtool_name
    E.g. tool = LegacyTool.objects.get(name='Subversion') to
    scmtool_name='Subversion'
    - Refactor RepositoryForm to use scmtool_name
    - Other misc refactoring to pass tests in scmtools, hostingsvcs,
    webapi, reviews, and diffviewer
~   - Add migratescmtools.py command.

  ~ - Add migratescmtools.py command.
  + - Add SCMToolHook extension and unit tests

Commit:

-f74803be8f01d11c78bc0b23e26f7ffcd62cf07a
+1d2bcb684ac2ee97fc0706f4683e8c4bb4c93da9

Diff:

Revision 8 (+3168 -2806)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

  1. 
      
  2. I'm wondering if this affects RBTools? I tried rbt patch to run the RBTools unit tests but applying the patch failed (Do I need to do DB changes?).

    rbtools/clients/__init__.py load_scmclients() is my guess on what might be affected, if anything at all...

    1. Unfortunately, I haven't worked with the rbtools repository, so I'm unclear on what problem you are encountering. Let me investigate a bit, and I will get back to you.

    2. (although none of my changes are on master, so I don't think it should be affecting you)

    3. RBTools does have SCM interfaces, but it's a completely separate infrastructure.

  3. 
      
  1. Overall great job! Looks good from what I can tell. Mostly just found docstring and spacing issues.

  2. reviewboard/scmtools/models.py (Diff revision 8)
     
     

    Has this been completed?

  3. reviewboard/scmtools/service.py (Diff revision 8)
     
     

    I think this should be tool?

    1. The type refers to the :py:class:SCMTool subclass.

  4. reviewboard/scmtools/service.py (Diff revision 8)
     
     

    Might need Args for name

  5. Can remove blank lines

  6. Spacing seemed fine before change

  7. 
      
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 10)
     
     
     

    These two lines should be switched so the imports are in alphabetical order.

  3. 
      
  1. 
      
  2. reviewboard/scmtools/service.py (Diff revision 10)
     
     

    cls is a "reserved" (emphasis on the quotes there) word. Consider changing the name? See @classmethods

  3. reviewboard/scmtools/service.py (Diff revision 10)
     
     

    See previous comment about cls (especially here used as an argument)

  4. 
      
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 10)
     
     

    setUp -> setup

    1. Yeah, this is a good point. However I'm following the style of the rest of the tests.py file, where setUp is used as well as test_register. But I will double check on Slack.

  3. reviewboard/extensions/tests.py (Diff revision 10)
     
     

    tearDown -> teardown

    1. Same reply as above.

  4. trailing white space

  5. What is reverse relation here?

    1. For example, in the Repository model there is a field _legacy_tool, which refers to an entry in the LegacyTool model. The "forward" relationship is to get the legacy tool given the repository (e.g. repository._legacy_tool).

      The reverse relation allows you to get all repositories given the legacy tool. To do this, you can set a related_name attribute for the foreign key. E.g. _legacy_tool = models.ForeignKey(LegacyTool, related_name='repositories', ...)

      So the reverse relation is getting all repositories given a legacy tool using tool.repositories.all().

  6. tearDown -> teardown

    1. Similar to tests.py the tests in scmtools/tests use both camel case and underscores, e.g. tearDown and test_registration

    2. Update: tearDown is defined by the superclass TestCase, and we are overriding it here. Thus the name cannot change.

  7. reviewboard/testing/testcase.py (Diff revision 10)
     
     

    setup_class()

    1. Similar comment as above.

    2. Update: setUpClass is defined by the superclass TestCase, and we are overriding it here. Thus the name cannot change.

  8. Could you explain what the q & Q does? I saw this elsewhere and was confused by the syntax.

    1. In this function, we want to build a query based on the attributes which the request has. To build this, we use the Q object. This object encapsulates field lookups.

      Higher up in the function, we set q = Q() and then do q & Q(...) to keep building the Q object.

      More about QuerySet: https://docs.djangoproject.com/en/2.0/topics/db/queries/
      More about Q(): https://docs.djangoproject.com/en/1.7/topics/db/queries/#complex-lookups-with-q

  9. 
      
Review request changed
Loading...