Summary

Add a registry for SCMTools.

Review Request #12333 — Created June 3, 2022 and submitted June 13, 2022, 3:16 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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:

If there's a VersionConflict in the install, no entry points will
load.
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.

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.	93d913d75f98628631cff94770f217e10aaa4061

Summary

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

Issues

Description	From	Last Updated
E501 line too long (82 > 79 characters)	reviewbot	June 3, 2022, 10:27 a.m.
Can you move these above Args? I think I did this, but they weren't supposed to be here. Also the …	chipx86	June 9, 2022, 12:35 p.m.
So it's mixed, but for the most part, we suffix our registries with _registry. I think that might be good …	chipx86	June 9, 2022, 12:37 p.m.
Do we need to do this? populate() should be called on first access to a registry.	chipx86	June 9, 2022, 12:36 p.m.
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 …	chipx86	June 9, 2022, 12:36 p.m.
We could save the final lookup by just loading everything for Tool up-front into an explicit list, adding anything we …	chipx86	June 9, 2022, 1 p.m.
We should use RemovedInReviewBoard060Warning.warn(...) rather than warnings. You can pass a stacklevel=1 if you want to use that.	chipx86	June 9, 2022, 12:48 p.m.
We should really require (or at least guide) registration and not entrypoints.	chipx86	June 9, 2022, 12:49 p.m.
This should be an explicit logging statement. Warnings aren't guaranteed to be shown or end up in the server log.	chipx86	June 9, 2022, 12:50 p.m.
This can be Tool.objects.create(). No need for .save().	chipx86	June 9, 2022, 12:51 p.m.
What does this raise?	chipx86	June 9, 2022, 12:51 p.m.
Same question here.	chipx86	June 9, 2022, 12:51 p.m.
'warnings' imported but unused Column: 1 Error code: F401	reviewbot	June 9, 2022, 1:35 p.m.
Small nit: Let's make this a set, so lookups in the block below are just a tad more efficient.	chipx86	June 13, 2022, 2:56 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/scmtools/registry.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (82 > 79 characters)
```

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.	3dffbef914884a0d073cf233fd26e5d3f2eb63be
	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

Diff:

Revision 2 (+522 -48)

Show changes

	setup.py
	docs/manual/docsmanage.py
	reviewboard/__init__.py
	reviewboard/scmtools/__init__.py
	reviewboard/scmtools/registry.py
	reviewboard/scmtools/tests/test_registry.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/__init__.py (Diff revision 2)

The issue has been resolved. 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.

reviewboard/scmtools/__init__.py (Diff revision 2)

The issue has been resolved. 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.

reviewboard/scmtools/__init__.py (Diff revision 2)

The issue has been dropped. Show all issues

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

david June 9, 2022, 1:23 p.m.

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.

chipx86 June 9, 2022, 1:32 p.m.

Ah that makes sense.

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

david June 9, 2022, 1:35 p.m.

Already made that change in the latest diff.

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

The issue has been dropped. 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.

david June 9, 2022, 1:23 p.m.

Let's make any changes like that separately.

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

The issue has been resolved. 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.

david June 9, 2022, 1:23 p.m.

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.

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

The issue has been resolved. Show all issues

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

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

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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

reviewboard/scmtools/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This can be Tool.objects.create(). No need for .save().
```

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

The issue has been resolved. Show all issues

What does this raise?

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

The issue has been resolved. Show all issues

Same question here.

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

Diff:

Revision 3 (+584 -54)

Show changes

	setup.py
	docs/manual/docsmanage.py
	reviewboard/__init__.py
	reviewboard/scmtools/__init__.py
	reviewboard/scmtools/apps.py
	reviewboard/scmtools/registry.py
	reviewboard/scmtools/tests/test_registry.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/scmtools/registry.py (Diff revision 3)
The issue has been dropped. Show all issues
```
'warnings' imported but unused

Column: 1
Error code: F401
```

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.	96749ce554eeebb1b95bbbe0b8b2369a0810e154
	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

Diff:

Revision 4 (+582 -54)

Show changes

	setup.py
	docs/manual/docsmanage.py
	reviewboard/__init__.py
	reviewboard/scmtools/__init__.py
	reviewboard/scmtools/apps.py
	reviewboard/scmtools/registry.py
	reviewboard/scmtools/tests/test_registry.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/scmtools/registry.py (Diff revision 4)

The issue has been resolved. Show all issues

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

Status:: Completed
Change Summary:: Pushed to release-5.0.x (2c72e82)