Summary

Fix issues upgrading SCMTool IDs on non-pristine setups.

Review Request #12380 — Created June 18, 2022 and submitted June 21, 2022, 4:48 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

These issues include:

Pre-upgrade only recorded state to upgrade if the
repository_scmtool_id was not yet in the database, preventing a
re-attempt at upgrading from working on failure (and outright
skipping the pre-upgrade work if running evolve --execute before
running a managed upgrade).
SCMTools that could not be loaded (missing package) would cause the
pre-upgrade steps to crash and no IDs to be migrated.
Integration configurations that lacked a saved conditions field
would cause a lookup on conditions to try to load the integration
class in order to retrieve the default, and as these are provided by
unloaded extensions, this would cause the pre-upgrade steps to
crash.

This change fixes all three scenarios.

Pre-upgrade steps for SCMTools are now run if either the evolution is
missing from the database or if we can find any repositories lacking
an ID. This enables re-attempts in both the premature evolve --execute
case and in the upgrade re-attempt case.

We now check for exceptions on getting the SCMTool IDs. If we fail to
load any, we'll list them and follow up with some helpful instructions.
These repositories will remain in their pre-upgrade state until resolved
(an upcoming change will improve upon this bad state).

For updates to integration configuration, the code is now careful to
work directly with the settings dictionary rather than the wrapper
methods, to avoid an attempted load of the integration. As an
optimization, the queryset also avoids loading any more state than
needed for the upgrade.

Testing Done

Tested an upgrade on a clean database.

Tested an upgrade on my messy database, with missing SCMTools and
integration configurations both without explicit conditions saved
and without a ConditionsField.

Verified that the IDs got migrated over even when errors were
encountered.

After resolving the errors, I verified that the upgrade could be
re-attempted and the remaining SCMTool IDs were looked up and
migrated over.

Commits

Summary	ID
Fix issues upgrading SCMTool IDs on non-pristine setups. While trying to upgrade my system to the new registry-backed SCMTool support, a number of issues with the new upgrade process for SCMTools 0were discovered, in part due to the complexity and history of my local database. These issues include: 1. Pre-upgrade only recorded state to upgrade if the `repository_scmtool_id` was not yet in the database, preventing a re-attempt at upgrading from working on failure (and outright skipping the pre-upgrade work if running `evolve --execute` before running a managed upgrade). 2. SCMTools that could not be loaded (missing package) would cause the pre-upgrade steps to crash and no IDs to be migrated. 3. Integration configurations that lacked a saved `conditions` field would cause a lookup on `conditions` to try to load the integration class in order to retrieve the default, and as these are provided by unloaded extensions, this would cause the pre-upgrade steps to crash. This change fixes all three scenarios. Pre-upgrade steps for SCMTools are now run if either the evolution is missing from the database or if we can find any repositories lacking an ID. This enables re-attempts in both the premature `evolve --execute` case and in the upgrade re-attempt case. We now check for exceptions on getting the SCMTool IDs. If we fail to load any, we'll list them and follow up with some helpful instructions. These repositories will remain in their pre-upgrade state until resolved (an upcoming change will improve upon this bad state). For updates to integration configuration, the code is now careful to work directly with the `settings` dictionary rather than the wrapper methods, to avoid an attempted load of the integration. As an optimization, the queryset also avoids loading any more state than needed for the upgrade.	1c87f7d931eebc34345eb9f3eefc5095ea053008

Summary

Fix issues upgrading SCMTool IDs on non-pristine setups.

While trying to upgrade my system to the new registry-backed SCMTool support, a number of issues with the new upgrade process for SCMTools 0were discovered, in part due to the complexity and history of my local database. These issues include: 1. Pre-upgrade only recorded state to upgrade if the `repository_scmtool_id` was not yet in the database, preventing a re-attempt at upgrading from working on failure (and outright skipping the pre-upgrade work if running `evolve --execute` before running a managed upgrade). 2. SCMTools that could not be loaded (missing package) would cause the pre-upgrade steps to crash and no IDs to be migrated. 3. Integration configurations that lacked a saved `conditions` field would cause a lookup on `conditions` to try to load the integration class in order to retrieve the default, and as these are provided by unloaded extensions, this would cause the pre-upgrade steps to crash. This change fixes all three scenarios. Pre-upgrade steps for SCMTools are now run if either the evolution is missing from the database *or* if we can find any repositories lacking an ID. This enables re-attempts in both the premature `evolve --execute` case and in the upgrade re-attempt case. We now check for exceptions on getting the SCMTool IDs. If we fail to load any, we'll list them and follow up with some helpful instructions. These repositories will remain in their pre-upgrade state until resolved (an upcoming change will improve upon this bad state). For updates to integration configuration, the code is now careful to work directly with the `settings` dictionary rather than the wrapper methods, to avoid an attempted load of the integration. As an optimization, the queryset also avoids loading any more state than needed for the upgrade.

1c87f7d931eebc34345eb9f3eefc5095ea053008

Issues

Description	From	Last Updated
This blank line isn't necessary.	david	June 21, 2022, 12:36 a.m.
I feel like we should print what tools are missing.	david	June 21, 2022, 2:34 a.m.
undefined name 'e' Column: 28 Error code: F821	reviewbot	June 21, 2022, 2:35 a.m.

flake8 passed.

JSHint passed.

reviewboard/upgrade.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This blank line isn't necessary.
```

reviewboard/upgrade.py (Diff revision 1)

Does this actually work when the scmtool_id field doesn't yet exist?

chipx86 June 21, 2022, 12:39 a.m.

No, but if has_evolution is False, we won't hit that code.

I could wrap the whole thing in an exception as an extra measure of protection.

reviewboard/upgrade.py (Diff revision 1)
The issue has been resolved. Show all issues
```
I feel like we should print what tools are missing.
```

Change Summary:


Removed a blank line.
Added extra protection in case a database query fails when determining whether to upgrade tools.
Changed the error output for tools that fail to load. This is built to be expanded in an upcoming change.

Commits:

	Summary	ID
	Fix issues upgrading SCMTool IDs on non-pristine setups. While trying to upgrade my system to the new registry-backed SCMTool support, a number of issues with the new upgrade process for SCMTools 0were discovered, in part due to the complexity and history of my local database. These issues include: 1. Pre-upgrade only recorded state to upgrade if the `repository_scmtool_id` was not yet in the database, preventing a re-attempt at upgrading from working on failure (and outright skipping the pre-upgrade work if running `evolve --execute` before running a managed upgrade). 2. SCMTools that could not be loaded (missing package) would cause the pre-upgrade steps to crash and no IDs to be migrated. 3. Integration configurations that lacked a saved `conditions` field would cause a lookup on `conditions` to try to load the integration class in order to retrieve the default, and as these are provided by unloaded extensions, this would cause the pre-upgrade steps to crash. This change fixes all three scenarios. Pre-upgrade steps for SCMTools are now run if either the evolution is missing from the database or if we can find any repositories lacking an ID. This enables re-attempts in both the premature `evolve --execute` case and in the upgrade re-attempt case. We now check for exceptions on getting the SCMTool IDs. If we fail to load any, we'll list them and follow up with some helpful instructions. These repositories will remain in their pre-upgrade state until resolved (an upcoming change will improve upon this bad state). For updates to integration configuration, the code is now careful to work directly with the `settings` dictionary rather than the wrapper methods, to avoid an attempted load of the integration. As an optimization, the queryset also avoids loading any more state than needed for the upgrade.	b546e75de0b11264a5ca4fec539097d255aa89b7
	Fix issues upgrading SCMTool IDs on non-pristine setups. While trying to upgrade my system to the new registry-backed SCMTool support, a number of issues with the new upgrade process for SCMTools 0were discovered, in part due to the complexity and history of my local database. These issues include: 1. Pre-upgrade only recorded state to upgrade if the `repository_scmtool_id` was not yet in the database, preventing a re-attempt at upgrading from working on failure (and outright skipping the pre-upgrade work if running `evolve --execute` before running a managed upgrade). 2. SCMTools that could not be loaded (missing package) would cause the pre-upgrade steps to crash and no IDs to be migrated. 3. Integration configurations that lacked a saved `conditions` field would cause a lookup on `conditions` to try to load the integration class in order to retrieve the default, and as these are provided by unloaded extensions, this would cause the pre-upgrade steps to crash. This change fixes all three scenarios. Pre-upgrade steps for SCMTools are now run if either the evolution is missing from the database or if we can find any repositories lacking an ID. This enables re-attempts in both the premature `evolve --execute` case and in the upgrade re-attempt case. We now check for exceptions on getting the SCMTool IDs. If we fail to load any, we'll list them and follow up with some helpful instructions. These repositories will remain in their pre-upgrade state until resolved (an upcoming change will improve upon this bad state). For updates to integration configuration, the code is now careful to work directly with the `settings` dictionary rather than the wrapper methods, to avoid an attempted load of the integration. As an optimization, the queryset also avoids loading any more state than needed for the upgrade.	2a7290c6b4553c6adfbc38a31a94c95717cb91a8

Diff:

Revision 2 (+250 -52)

Show changes

reviewboard/upgrade.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
```
undefined name 'e'

Column: 28
Error code: F821
```

Change Summary:

Fixed an undefined variable in an exception handler.

Commits:

	Summary	ID
	Fix issues upgrading SCMTool IDs on non-pristine setups. While trying to upgrade my system to the new registry-backed SCMTool support, a number of issues with the new upgrade process for SCMTools 0were discovered, in part due to the complexity and history of my local database. These issues include: 1. Pre-upgrade only recorded state to upgrade if the `repository_scmtool_id` was not yet in the database, preventing a re-attempt at upgrading from working on failure (and outright skipping the pre-upgrade work if running `evolve --execute` before running a managed upgrade). 2. SCMTools that could not be loaded (missing package) would cause the pre-upgrade steps to crash and no IDs to be migrated. 3. Integration configurations that lacked a saved `conditions` field would cause a lookup on `conditions` to try to load the integration class in order to retrieve the default, and as these are provided by unloaded extensions, this would cause the pre-upgrade steps to crash. This change fixes all three scenarios. Pre-upgrade steps for SCMTools are now run if either the evolution is missing from the database or if we can find any repositories lacking an ID. This enables re-attempts in both the premature `evolve --execute` case and in the upgrade re-attempt case. We now check for exceptions on getting the SCMTool IDs. If we fail to load any, we'll list them and follow up with some helpful instructions. These repositories will remain in their pre-upgrade state until resolved (an upcoming change will improve upon this bad state). For updates to integration configuration, the code is now careful to work directly with the `settings` dictionary rather than the wrapper methods, to avoid an attempted load of the integration. As an optimization, the queryset also avoids loading any more state than needed for the upgrade.	2a7290c6b4553c6adfbc38a31a94c95717cb91a8
	Fix issues upgrading SCMTool IDs on non-pristine setups. While trying to upgrade my system to the new registry-backed SCMTool support, a number of issues with the new upgrade process for SCMTools 0were discovered, in part due to the complexity and history of my local database. These issues include: 1. Pre-upgrade only recorded state to upgrade if the `repository_scmtool_id` was not yet in the database, preventing a re-attempt at upgrading from working on failure (and outright skipping the pre-upgrade work if running `evolve --execute` before running a managed upgrade). 2. SCMTools that could not be loaded (missing package) would cause the pre-upgrade steps to crash and no IDs to be migrated. 3. Integration configurations that lacked a saved `conditions` field would cause a lookup on `conditions` to try to load the integration class in order to retrieve the default, and as these are provided by unloaded extensions, this would cause the pre-upgrade steps to crash. This change fixes all three scenarios. Pre-upgrade steps for SCMTools are now run if either the evolution is missing from the database or if we can find any repositories lacking an ID. This enables re-attempts in both the premature `evolve --execute` case and in the upgrade re-attempt case. We now check for exceptions on getting the SCMTool IDs. If we fail to load any, we'll list them and follow up with some helpful instructions. These repositories will remain in their pre-upgrade state until resolved (an upcoming change will improve upon this bad state). For updates to integration configuration, the code is now careful to work directly with the `settings` dictionary rather than the wrapper methods, to avoid an attempted load of the integration. As an optimization, the queryset also avoids loading any more state than needed for the upgrade.	1c87f7d931eebc34345eb9f3eefc5095ea053008

Diff:

Revision 3 (+250 -52)

Show changes

reviewboard/upgrade.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

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