Add a registry for SCMTools.

Review Request #12333 — Created June 3, 2022 and submitted

Information

Review Board
release-5.0.x

Reviewers

The way that SCMTools are kept track of is particularly old. From the
very earliest days of Review Board, we knew that we wanted to support
multiple different SCMs, and we had done this by creating a Tool model
which had a name and the module path of a class to instantiate. This was
populated via database fixtures, and Repository then had a relation to
the tool.

As we improved things and made everything extensible, we moved away from
fixtures and over to entry points, but this still had two major
problems:

  1. If there's a VersionConflict in the install, no entry points will
    load.
  2. There was still a manual step that had to happen to load the tool
    types from the entry points and populate the Tool table.
    Occasionally, usually due to problem #1, people would end up with an
    install that didn't have any Tool models, and we'd have to direct
    them to manually run the registerscmtools command.

This change is the first major step towards modernizing this entire part
of the system. This adds a new registry for storing the SCMTool classes.
By default, this is populated from three places. First, we load the
built-in SCMTools (whether or not entry points can load). Second, we
load anything present in entry points. Finally, we'll go through the
existing Tool table, and load anything else from there (while printing a
warning that said table is going away, and anything there will need to
be changed to use entry points or the register() method directly.

  • Checked that the registry was properly populated from built-ins and
    entry points.
  • Created a fake item in the tool table and populated the registry. Saw
    that it was added to the registry and the appropriate warning was
    raised.
  • Removed an item from the Tool table. Re-populated the registry and saw
    that the Tool instance was properly created.
  • Ran unit tests.
Summary ID
Add a registry for SCMTools.
The way that SCMTools are kept track of is particularly old. From the very earliest days of Review Board, we knew that we wanted to support multiple different SCMs, and we had done this by creating a `Tool` model which had a name and the module path of a class to instantiate. This was populated via database fixtures, and `Repository` then had a relation to the tool. As we improved things and made everything extensible, we moved away from fixtures and over to entry points, but this still had two major problems: 1. If there's a `VersionConflict` in the install, no entry points will load. 2. There was still a manual step that had to happen to load the tool types from the entry points and populate the `Tool` table. Occasionally, usually due to problem #1, people would end up with an install that didn't have any Tool models, and we'd have to direct them to manually run the `registerscmtools` command. This change is the first major step towards modernizing this entire part of the system. This adds a new registry for storing the SCMTool classes. By default, this is populated from three places. First, we load the built-in SCMTools (whether or not entry points can load). Second, we load anything present in entry points. Finally, we'll go through the existing Tool table, and load anything else from there (while printing a warning that said table is going away, and anything there will need to be changed to use entry points or the `register()` method directly. Testing Done: - Checked that the registry was properly populated from built-ins and entry points. - Created a fake item in the tool table and populated the registry. Saw that it was added to the registry and the appropriate warning was raised. - Removed an item from the Tool table. Re-populated the registry and saw that the Tool instance was properly created. - Ran unit tests.
93d913d75f98628631cff94770f217e10aaa4061
Description From Last Updated

E501 line too long (82 > 79 characters)

reviewbotreviewbot

Can you move these above Args? I think I did this, but they weren't supposed to be here. Also the …

chipx86chipx86

So it's mixed, but for the most part, we suffix our registries with _registry. I think that might be good …

chipx86chipx86

Do we need to do this? populate() should be called on first access to a registry.

chipx86chipx86

Long-term, I'd love to nuke the entrypoint support here. They're more problematic than anything, and it's better to control instantiation/registration …

chipx86chipx86

We could save the final lookup by just loading everything for Tool up-front into an explicit list, adding anything we …

chipx86chipx86

We should use RemovedInReviewBoard060Warning.warn(...) rather than warnings. You can pass a stacklevel=1 if you want to use that.

chipx86chipx86

We should really require (or at least guide) registration and not entrypoints.

chipx86chipx86

This should be an explicit logging statement. Warnings aren't guaranteed to be shown or end up in the server log.

chipx86chipx86

This can be Tool.objects.create(). No need for .save().

chipx86chipx86

What does this raise?

chipx86chipx86

Same question here.

chipx86chipx86

'warnings' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

Small nit: Let's make this a set, so lookups in the block below are just a tad more efficient.

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

flake8

david
chipx86
  1. 
      
  2. reviewboard/__init__.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    Can you move these above Args? I think I did this, but they weren't supposed to be here.

    Also the 4.0 shouldn't have a : after it, as the parser will expect a second line with a description.

    No indentation on that description line for 5.0.

    Also should be in order of newest to oldest.

  3. reviewboard/scmtools/__init__.py (Diff revision 2)
     
     
    Show all issues

    So it's mixed, but for the most part, we suffix our registries with _registry. I think that might be good to do here, especially given that this is the same name as the module it's in.

  4. reviewboard/scmtools/__init__.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    Do we need to do this? populate() should be called on first access to a registry.

    1. I want to keep this, because populating the registry has the side-effect of also populating anything that's missing into the Tool table. While we won't be using that model anymore, it's possible that third-party code still will.

    2. Ah that makes sense.

      In that case, can we connect the signal in the app's ready handler? This avoids side-effects on import.

    3. Already made that change in the latest diff.

  5. reviewboard/scmtools/registry.py (Diff revision 2)
     
     
    Show all issues

    Long-term, I'd love to nuke the entrypoint support here. They're more problematic than anything, and it's better to control instantiation/registration through extensions.

    Short-term, I know we need it.

    If you're on board with the long-term goal, can we document in the docstring that entrypoint support is deprecated? We'd also need to emit a deprecation warning on anything we load.

    1. Let's make any changes like that separately.

  6. reviewboard/scmtools/registry.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    We could save the final lookup by just loading everything for Tool up-front into an explicit list, adding anything we create to that list, and then iterating over the result.

    1. I can actually make it just do the full query once at the beginning since any new tools by definition won't exist in the DB but not the registry.

  7. reviewboard/scmtools/registry.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    We should use RemovedInReviewBoard060Warning.warn(...) rather than warnings.

    You can pass a stacklevel=1 if you want to use that.

  8. reviewboard/scmtools/registry.py (Diff revision 2)
     
     
     
     
    Show all issues

    We should really require (or at least guide) registration and not entrypoints.

  9. reviewboard/scmtools/registry.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    This should be an explicit logging statement. Warnings aren't guaranteed to be shown or end up in the server log.

  10. reviewboard/scmtools/registry.py (Diff revision 2)
     
     
     
     
    Show all issues

    This can be Tool.objects.create(). No need for .save().

  11. reviewboard/scmtools/registry.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    What does this raise?

  12. reviewboard/scmtools/registry.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Same question here.

  13. 
      
david
Review request changed
Change Summary:
  • Make requested changes
  • Move registry population into an AppConfig.ready method instead of using initializing.
Commits:
Summary ID
Add a registry for SCMTools.
The way that SCMTools are kept track of is particularly old. From the very earliest days of Review Board, we knew that we wanted to support multiple different SCMs, and we had done this by creating a `Tool` model which had a name and the module path of a class to instantiate. This was populated via database fixtures, and `Repository` then had a relation to the tool. As we improved things and made everything extensible, we moved away from fixtures and over to entry points, but this still had two major problems: 1. If there's a `VersionConflict` in the install, no entry points will load. 2. There was still a manual step that had to happen to load the tool types from the entry points and populate the `Tool` table. Occasionally, usually due to problem #1, people would end up with an install that didn't have any Tool models, and we'd have to direct them to manually run the `registerscmtools` command. This change is the first major step towards modernizing this entire part of the system. This adds a new registry for storing the SCMTool classes. By default, this is populated from three places. First, we load the built-in SCMTools (whether or not entry points can load). Second, we load anything present in entry points. Finally, we'll go through the existing Tool table, and load anything else from there (while printing a warning that said table is going away, and anything there will need to be changed to use entry points or the `register()` method directly. Testing Done: - Checked that the registry was properly populated from built-ins and entry points. - Created a fake item in the tool table and populated the registry. Saw that it was added to the registry and the appropriate warning was raised. - Removed an item from the Tool table. Re-populated the registry and saw that the Tool instance was properly created. - Ran unit tests.
d775618bcecdb1e0fd5d5ef270066831a3e16f5f
Add a registry for SCMTools.
The way that SCMTools are kept track of is particularly old. From the very earliest days of Review Board, we knew that we wanted to support multiple different SCMs, and we had done this by creating a `Tool` model which had a name and the module path of a class to instantiate. This was populated via database fixtures, and `Repository` then had a relation to the tool. As we improved things and made everything extensible, we moved away from fixtures and over to entry points, but this still had two major problems: 1. If there's a `VersionConflict` in the install, no entry points will load. 2. There was still a manual step that had to happen to load the tool types from the entry points and populate the `Tool` table. Occasionally, usually due to problem #1, people would end up with an install that didn't have any Tool models, and we'd have to direct them to manually run the `registerscmtools` command. This change is the first major step towards modernizing this entire part of the system. This adds a new registry for storing the SCMTool classes. By default, this is populated from three places. First, we load the built-in SCMTools (whether or not entry points can load). Second, we load anything present in entry points. Finally, we'll go through the existing Tool table, and load anything else from there (while printing a warning that said table is going away, and anything there will need to be changed to use entry points or the `register()` method directly. Testing Done: - Checked that the registry was properly populated from built-ins and entry points. - Created a fake item in the tool table and populated the registry. Saw that it was added to the registry and the appropriate warning was raised. - Removed an item from the Tool table. Re-populated the registry and saw that the Tool instance was properly created. - Ran unit tests.
96749ce554eeebb1b95bbbe0b8b2369a0810e154

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. reviewboard/scmtools/registry.py (Diff revision 4)
     
     
    Show all issues

    Small nit: Let's make this a set, so lookups in the block below are just a tad more efficient.

  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.0.x (2c72e82)