Add a registry for SCMTools.

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

david
Review Board
release-5.0.x
reviewboard

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
Add a registry for SCMTools.
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)
     
     
     
     
     
     
     

    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)
     
     

    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)
     
     
     
     
     

    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)
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     

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

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

    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)
     
     
     
     

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

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

    What does this raise?

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

    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
-
Add a registry for SCMTools.
+
Add a registry for SCMTools.

Diff:

Revision 3 (+584 -54)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (2c72e82)
Loading...