Refactor SCM tool registration using a registry

Review Request #9724 — Created March 3, 2018 and discarded

Information

Review Board
master

Reviewers

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.

Description From Last Updated

I'm wondering if this affects RBTools? I tried rbt patch to run the RBTools unit tests but applying the patch …

MA maram

There should be one empty line after the doc string.

RI ridum

There should be one empty line after the doc string.

RI ridum

We need a whitespace between the doc String and the code

RI ridum

There should be one empty line after the doc string.

RI ridum

only one empty line is needed after yield

RI ridum

F401 'pkg_resources' imported but unused

reviewbotreviewbot

F401 'sys' imported but unused

reviewbotreviewbot

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

RI ridum

There should be one empty line after the doc string.

RI ridum

NOTE: For some reason, the diff captured changes in whitespace which is why there are so many line chaneges in …

JT jtrang

Any specific reason to use " over '?

RO rodgerwa

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

JC jcorrive

Is this commented line of code here for a purpose?

JC jcorrive

F401 'reviewboard.scmtools.service.unregister_scm_tool' imported but unused

reviewbotreviewbot

F401 'reviewboard.scmtools.service.register_scm_tool' imported but unused

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

Has this been completed?

JQ jque

I think this should be tool?

JQ jque

Might need Args for name

JQ jque

Can remove blank lines

JQ jque

Spacing seemed fine before change

JQ jque

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

JC jcorrive

setUp -> setup

FL floriecai

tearDown -> teardown

FL floriecai

trailing white space

FL floriecai

What is reverse relation here?

FL floriecai

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

rpolyanorpolyano

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

rpolyanorpolyano

tearDown -> teardown

FL floriecai

setup_class()

FL floriecai

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

FL floriecai
JT
JT
JT
JT
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

JT
JT
JT
  1. 
      
  2. docs/manual/fixtures/initial_data.json (Diff revision 5)
     
     
    Show all issues

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

    Any specific reason to use " over '?

    1. Thanks! Good catch.

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

    There should be one empty line after the doc string.

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

    There should be one empty line after the doc string.

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

    We need a whitespace between the doc String and the code

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

    There should be one empty line after the doc string.

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

    only one empty line is needed after yield

  7. Show all issues

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

    There should be one empty line after the doc string.

  9. 
      
JC
  1. 
      
  2. reviewboard/scmtools/service.py (Diff revision 5)
     
     
    Show all issues

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

  3. Show all issues

    Is this commented line of code here for a purpose?

  4. 
      
JT
JT
JT
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MA
  1. 
      
  2. Show all issues

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

    Has this been completed?

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

    I think this should be tool?

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

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

    Might need Args for name

  5. Show all issues

    Can remove blank lines

  6. Show all issues

    Spacing seemed fine before change

  7. 
      
JT
JT
JT
JC
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 10)
     
     
     
    Show all issues

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

  3. 
      
rpolyano
  1. 
      
  2. reviewboard/scmtools/service.py (Diff revision 10)
     
     
    Show all issues

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

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

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

  4. 
      
FL
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 10)
     
     
    Show all issues

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

    tearDown -> teardown

    1. Same reply as above.

  4. Show all issues

    trailing white space

  5. Show all issues

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

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

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

    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. 
      
JT
david
Review request changed
Status:
Discarded