Summary

Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.

Review Request #12336 — 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

One of the last places where we were using the old Tool model was the
"Repository Type" condition for integration configurations. This used
the BaseConditionModelMultipleChoice to show a multiple choice field
with the various registered repository types. This meant that the form
field was populated using the contents of the Tool table, and selected
values were stored as the primary keys of the selected Tool instances.

This change makes it so the RepositoryTypeChoice instead loads its
values from the new SCMTools registry, using the SCMTool ID as the key.
In order to handle any existing integration configurations, a new stage
has been added to the upgrade to look through existing integration
configurations and update them.

Testing Done


Created and edited integration configurations using various usage of

  the "Repository Type" choice. Saw that everything was loaded from and

  saved to the database correctly.
Created several integration configurations using the "Repository Type"

  choice and then ran the upgrade. Saw that old lists of integers

  (Tool PKs) were updated to be lists of strings corresponding to the

  correct SCMTool IDs.
Ran unit tests.

Commits

Summary	ID
Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK. One of the last places where we were using the old `Tool` model was the "Repository Type" condition for integration configurations. This used the `BaseConditionModelMultipleChoice` to show a multiple choice field with the various registered repository types. This meant that the form field was populated using the contents of the Tool table, and selected values were stored as the primary keys of the selected `Tool` instances. This change makes it so the `RepositoryTypeChoice` instead loads its values from the new SCMTools registry, using the SCMTool ID as the key. In order to handle any existing integration configurations, a new stage has been added to the upgrade to look through existing integration configurations and update them. Testing Done: - Created and edited integration configurations using various usage of the "Repository Type" choice. Saw that everything was loaded from and saved to the database correctly. - Created several integration configurations using the "Repository Type" choice and then ran the upgrade. Saw that old lists of integers (`Tool` PKs) were updated to be lists of strings corresponding to the correct SCMTool IDs. - Ran unit tests.	988b3c28ef3388f4e00259b2a1a596fb5a10368a

Summary

Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.

One of the last places where we were using the old `Tool` model was the "Repository Type" condition for integration configurations. This used the `BaseConditionModelMultipleChoice` to show a multiple choice field with the various registered repository types. This meant that the form field was populated using the contents of the Tool table, and selected values were stored as the primary keys of the selected `Tool` instances. This change makes it so the `RepositoryTypeChoice` instead loads its values from the new SCMTools registry, using the SCMTool ID as the key. In order to handle any existing integration configurations, a new stage has been added to the upgrade to look through existing integration configurations and update them. Testing Done: - Created and edited integration configurations using various usage of the "Repository Type" choice. Saw that everything was loaded from and saved to the database correctly. - Created several integration configurations using the "Repository Type" choice and then ran the upgrade. Saw that old lists of integers (`Tool` PKs) were updated to be lists of strings corresponding to the correct SCMTool IDs. - Ran unit tests.

988b3c28ef3388f4e00259b2a1a596fb5a10368a

Issues

Description	From	Last Updated
Just realized, here and the other data migration change, that scmtool_id can fail if we can't load the SCMTool for …	chipx86	June 10, 2022, 1:04 p.m.
Two things we can do to optimize this: Skip saving if we don't find a condition to update. Call config.save(update_fields=('settings',)) …	chipx86	June 9, 2022, 2:49 p.m.
We should make the lookup conditional, since it's completely possible that we'll fail to find a matching value.	chipx86	June 10, 2022, 1:04 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	June 10, 2022, 1:05 p.m.
local variable 'updated_conditions' is assigned to but never used Column: 21 Error code: F841	reviewbot	June 10, 2022, 1:05 p.m.

flake8 passed.

JSHint passed.

reviewboard/upgrade.py (Diff revision 1)

The issue has been resolved. Show all issues

Just realized, here and the other data migration change, that scmtool_id can fail if we can't load the SCMTool for any reason (extension not installed, legacy tool, etc.).

We should check for this and warn with some sort of instructions. I don't know what that'd look like.

david June 9, 2022, 2:49 p.m.

In that case, the upgrade procedure should fail due to an ImproperlyConfigured getting raised from Tool.get_scmtool_class. I'd be pretty shocked if they hit it here but haven't already hit it in many other places.

That said, perhaps I can go through the existing conditions and just ignore any tools which aren't used there?

chipx86 June 10, 2022, 4:28 p.m.

Maybe.

I hit similar sorts of issues when doing SOS development. Ended up with a database containing the SOS SCMTool and repositories using it, but then used it on a new virtualenv without that version of Power Pack. Things blew up.

There's probably a lot that needs to be done here, but it's worth considering as we extend what's in Power Pack.

reviewboard/upgrade.py (Diff revision 1)

The issue has been resolved. Show all issues

Two things we can do to optimize this:

Skip saving if we don't find a condition to update.
Call config.save(update_fields=('settings',))

Also doesn't look like i or enumerate is needed anymore.

reviewboard/upgrade.py (Diff revision 1)

The issue has been resolved. Show all issues

We should make the lookup conditional, since it's completely possible that we'll fail to find a matching value.

Commits:

	Summary	ID
	Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK. One of the last places where we were using the old `Tool` model was the "Repository Type" condition for integration configurations. This used the `BaseConditionModelMultipleChoice` to show a multiple choice field with the various registered repository types. This meant that the form field was populated using the contents of the Tool table, and selected values were stored as the primary keys of the selected `Tool` instances. This change makes it so the `RepositoryTypeChoice` instead loads its values from the new SCMTools registry, using the SCMTool ID as the key. In order to handle any existing integration configurations, a new stage has been added to the upgrade to look through existing integration configurations and update them. Testing Done: - Created and edited integration configurations using various usage of the "Repository Type" choice. Saw that everything was loaded from and saved to the database correctly. - Created several integration configurations using the "Repository Type" choice and then ran the upgrade. Saw that old lists of integers (`Tool` PKs) were updated to be lists of strings corresponding to the correct SCMTool IDs. - Ran unit tests.	9419a349b1a3592b963cc930edc79b6dc973e659
	Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK. One of the last places where we were using the old `Tool` model was the "Repository Type" condition for integration configurations. This used the `BaseConditionModelMultipleChoice` to show a multiple choice field with the various registered repository types. This meant that the form field was populated using the contents of the Tool table, and selected values were stored as the primary keys of the selected `Tool` instances. This change makes it so the `RepositoryTypeChoice` instead loads its values from the new SCMTools registry, using the SCMTool ID as the key. In order to handle any existing integration configurations, a new stage has been added to the upgrade to look through existing integration configurations and update them. Testing Done: - Created and edited integration configurations using various usage of the "Repository Type" choice. Saw that everything was loaded from and saved to the database correctly. - Created several integration configurations using the "Repository Type" choice and then ran the upgrade. Saw that old lists of integers (`Tool` PKs) were updated to be lists of strings corresponding to the correct SCMTool IDs. - Ran unit tests.	3c386549dec7713f8ffe85e984bc4a3c04148372

Diff:

Revision 2 (+220 -100)

Show changes

	reviewboard/upgrade.py
	reviewboard/reviews/tests/test_conditions.py
	reviewboard/scmtools/conditions.py
	reviewboard/scmtools/management/commands/registerscmtools.py
	reviewboard/scmtools/tests/test_conditions.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/upgrade.py (Diff revision 2)
The issue has been resolved. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```

reviewboard/upgrade.py (Diff revision 2)

The issue has been dropped. Show all issues

local variable 'updated_conditions' is assigned to but never used

Column: 21
Error code: F841

Commits:

	Summary	ID
	Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK. One of the last places where we were using the old `Tool` model was the "Repository Type" condition for integration configurations. This used the `BaseConditionModelMultipleChoice` to show a multiple choice field with the various registered repository types. This meant that the form field was populated using the contents of the Tool table, and selected values were stored as the primary keys of the selected `Tool` instances. This change makes it so the `RepositoryTypeChoice` instead loads its values from the new SCMTools registry, using the SCMTool ID as the key. In order to handle any existing integration configurations, a new stage has been added to the upgrade to look through existing integration configurations and update them. Testing Done: - Created and edited integration configurations using various usage of the "Repository Type" choice. Saw that everything was loaded from and saved to the database correctly. - Created several integration configurations using the "Repository Type" choice and then ran the upgrade. Saw that old lists of integers (`Tool` PKs) were updated to be lists of strings corresponding to the correct SCMTool IDs. - Ran unit tests.	3c386549dec7713f8ffe85e984bc4a3c04148372
	Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK. One of the last places where we were using the old `Tool` model was the "Repository Type" condition for integration configurations. This used the `BaseConditionModelMultipleChoice` to show a multiple choice field with the various registered repository types. This meant that the form field was populated using the contents of the Tool table, and selected values were stored as the primary keys of the selected `Tool` instances. This change makes it so the `RepositoryTypeChoice` instead loads its values from the new SCMTools registry, using the SCMTool ID as the key. In order to handle any existing integration configurations, a new stage has been added to the upgrade to look through existing integration configurations and update them. Testing Done: - Created and edited integration configurations using various usage of the "Repository Type" choice. Saw that everything was loaded from and saved to the database correctly. - Created several integration configurations using the "Repository Type" choice and then ran the upgrade. Saw that old lists of integers (`Tool` PKs) were updated to be lists of strings corresponding to the correct SCMTool IDs. - Ran unit tests.	988b3c28ef3388f4e00259b2a1a596fb5a10368a

Diff:

Revision 3 (+218 -100)

Show changes

	reviewboard/upgrade.py
	reviewboard/reviews/tests/test_conditions.py
	reviewboard/scmtools/conditions.py
	reviewboard/scmtools/management/commands/registerscmtools.py
	reviewboard/scmtools/tests/test_conditions.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

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