Convert RepositoryTypeCondition to store SCMTool ID instead of Tool PK.

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

Information

Review Board
release-5.0.x

Reviewers

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.

  • 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.
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
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 …

chipx86chipx86

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',)) …

chipx86chipx86

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

chipx86chipx86

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

local variable 'updated_conditions' is assigned to but never used Column: 21 Error code: F841

reviewbotreviewbot
chipx86
  1. 
      
  2. reviewboard/upgrade.py (Diff revision 1)
     
     
     
     
     

    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.

    1. 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?

    2. 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.

  3. reviewboard/upgrade.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     

    Two things we can do to optimize this:

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

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

  4. reviewboard/upgrade.py (Diff revision 1)
     
     
     

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

  5. 
      
david
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

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