Summary

Fix upgrading oauth2_provider and accounts.Profile on Review Board 5.

Review Request #12281 — Created May 15, 2022 and submitted May 15, 2022, 10:32 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Our big move to Django 3.2 has been blocked largely due the
oauth2_provider app. In Review Board 4, we switched this from our
in-house evolutions to the official migrations, with the belief that
that would be the better upgrade path going forward.

Unfortunately, at some point, that project reset their baseline
migrations, meaning that it was no longer possible to upgrade the app
without just starting over on the schema or doing some manual
workarounds. This is not an option for us.

Work was explored on giving Django Evolution the ability to override
migrations, and then on undoing a MoveToMigrations mutation. In the
end, the best solution was just to give our own upgrade code a
pre-upgrade step of modifying the stored evolution and migration state,
undoing the MoveToMigrations ourselves, and then overriding the
evolutions used for this app.

This is now done in a new reviewboard.upgrade module, which has
pre-upgrade and post-upgrade steps that can be applied by both
manage.py and rb-site upgrade.

The pre-upgrade step deletes the migration and evolution history for
oauth2_provider and modifies the stored signature. That then allows
our custom evolutions to apply on top of it. If this is a completely new
install, none of this kicks in, but Django Evolution will proceed with
the standard evolution process for creating the models for the app,
marking those evolutions as applied and setting us up for future
evolutions to this app down the road.

There's also a fix for the Profile model, which lacked evolutions for
one of the changes made for 5.0.

Testing Done

Tested upgrading from 3.0 and from 4.0 to 5.0 on SQLite.

Tested a brand new install of 5.0 on SQLite.

Tests are still pending for earlier versions of Review Board (which
are failing, but are unrelated to this, and likely regressions in
Django Evolution) and for MySQL and Postgres databases.

Commits

Summary	ID
Fix upgrading oauth2_provider and accounts.Profile on Review Board 5. Our big move to Django 3.2 has been blocked largely due the `oauth2_provider` app. In Review Board 4, we switched this from our in-house evolutions to the official migrations, with the belief that that would be the better upgrade path going forward. Unfortunately, at some point, that project reset their baseline migrations, meaning that it was no longer possible to upgrade the app without just starting over on the schema or doing some manual workarounds. This is not an option for us. Work was explored on giving Django Evolution the ability to override migrations, and then on undoing a `MoveToMigrations` mutation. In the end, the best solution was just to give our own upgrade code a pre-upgrade step of modifying the stored evolution and migration state, undoing the `MoveToMigrations` ourselves, and then overriding the evolutions used for this app. This is now done in a new `reviewboard.upgrade` module, which has pre-upgrade and post-upgrade steps that can be applied by both `manage.py` and `rb-site upgrade`. The pre-upgrade step deletes the migration and evolution history for `oauth2_provider` and modifies the stored signature. That then allows our custom evolutions to apply on top of it. If this is a completely new install, none of this kicks in, but Django Evolution will proceed with the standard evolution process for creating the models for the app, marking those evolutions as applied and setting us up for future evolutions to this app down the road. There's also a fix for the `Profile` model, which lacked evolutions for one of the changes made for 5.0.	28ae86ed0d6fae1cfbce762b25c07d3718acd357

Summary

Fix upgrading oauth2_provider and accounts.Profile on Review Board 5.

Our big move to Django 3.2 has been blocked largely due the `oauth2_provider` app. In Review Board 4, we switched this from our in-house evolutions to the official migrations, with the belief that that would be the better upgrade path going forward. Unfortunately, at some point, that project reset their baseline migrations, meaning that it was no longer possible to upgrade the app without just starting over on the schema or doing some manual workarounds. This is not an option for us. Work was explored on giving Django Evolution the ability to override migrations, and then on undoing a `MoveToMigrations` mutation. In the end, the best solution was just to give our own upgrade code a pre-upgrade step of modifying the stored evolution and migration state, undoing the `MoveToMigrations` ourselves, and then overriding the evolutions used for this app. This is now done in a new `reviewboard.upgrade` module, which has pre-upgrade and post-upgrade steps that can be applied by both `manage.py` and `rb-site upgrade`. The pre-upgrade step deletes the migration and evolution history for `oauth2_provider` and modifies the stored signature. That then allows our custom evolutions to apply on top of it. If this is a completely new install, none of this kicks in, but Django Evolution will proceed with the standard evolution process for creating the models for the app, marking those evolutions as applied and setting us up for future evolutions to this app down the road. There's also a fix for the `Profile` model, which lacked evolutions for one of the changes made for 5.0.

28ae86ed0d6fae1cfbce762b25c07d3718acd357

Issues

Description	From	Last Updated
Module docstring.	david	May 15, 2022, 9:36 p.m.
Module docstring.	david	May 15, 2022, 9:38 p.m.
Module docstring.	david	May 15, 2022, 9:39 p.m.
F401 'django_evolution.mutations.ChangeMeta' imported but unused	reviewbot	May 15, 2022, 3:26 p.m.
What's the reason to pass in upgrade_state here. Can't we have this just return the new state dict?	david	May 15, 2022, 9:39 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/oauth/evolutions/upgrade_to_1_6_3.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'django_evolution.mutations.ChangeMeta' imported but unused
```

reviewboard/accounts/evolutions/profile_default_use_rich_text_boolean_field.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Module docstring.
```
reviewboard/admin/custom_evolutions/oauth2_provider/upgrade_to_1_6_3.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Module docstring.
```
reviewboard/oauth/evolutions/upgrade_to_1_6_3.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Module docstring.
```

reviewboard/upgrade.py (Diff revision 1)

The issue has been resolved. Show all issues

What's the reason to pass in upgrade_state here. Can't we have this just return the new state dict?

chipx86 May 15, 2022, 4:15 p.m.

I wanted to have a larger discussion about the design here, since we're both working on upgrade code, but here's my thoughts on this.
We definitely want a way for the pre-upgrade tasks to store state. They could return a dict or run_pre_upgrade_tasks() could create and return the dict. However, I wanted to be able to have both rb-site and manage.py pass in state they've already computed, such as the old and new version numbers.
These could be parameters to run_pre_upgrade_tasks(), which get populated in a dict, but my instinct was to have this function just be a task runner and let the dict be built elsewhere. Maybe another function in upgrade.py could take in things like the versions and build the dict. Maybe the caller does it. I don't know. Haven't had as much time to really look over what we're both doing and the old code and think about it, so I just figured keep it simple for now so it's easy to change.

Change Summary:


Added module docstrings.
Removed the old move_to_migrations.py evolution.
Moved imports in reviewboard.upgrade into the function that uses them.
Added back the evolution dependency in reviewboard.oauth.evolutions.

Commits:

	Summary	ID
	Fix upgrading oauth2_provider and accounts.Profile on Review Board 5. Our big move to Django 3.2 has been blocked largely due the `oauth2_provider` app. In Review Board 4, we switched this from our in-house evolutions to the official migrations, with the belief that that would be the better upgrade path going forward. Unfortunately, at some point, that project reset their baseline migrations, meaning that it was no longer possible to upgrade the app without just starting over on the schema or doing some manual workarounds. This is not an option for us. Work was explored on giving Django Evolution the ability to override migrations, and then on undoing a `MoveToMigrations` mutation. In the end, the best solution was just to give our own upgrade code a pre-upgrade step of modifying the stored evolution and migration state, undoing the `MoveToMigrations` ourselves, and then overriding the evolutions used for this app. This is now done in a new `reviewboard.upgrade` module, which has pre-upgrade and post-upgrade steps that can be applied by both `manage.py` and `rb-site upgrade`. The pre-upgrade step deletes the migration and evolution history for `oauth2_provider` and modifies the stored signature. That then allows our custom evolutions to apply on top of it. If this is a completely new install, none of this kicks in, but Django Evolution will proceed with the standard evolution process for creating the models for the app, marking those evolutions as applied and setting us up for future evolutions to this app down the road. There's also a fix for the `Profile` model, which lacked evolutions for one of the changes made for 5.0.	f5fe24dc18721dc86e4652e4cfd095ee8be50fc9
	Fix upgrading oauth2_provider and accounts.Profile on Review Board 5. Our big move to Django 3.2 has been blocked largely due the `oauth2_provider` app. In Review Board 4, we switched this from our in-house evolutions to the official migrations, with the belief that that would be the better upgrade path going forward. Unfortunately, at some point, that project reset their baseline migrations, meaning that it was no longer possible to upgrade the app without just starting over on the schema or doing some manual workarounds. This is not an option for us. Work was explored on giving Django Evolution the ability to override migrations, and then on undoing a `MoveToMigrations` mutation. In the end, the best solution was just to give our own upgrade code a pre-upgrade step of modifying the stored evolution and migration state, undoing the `MoveToMigrations` ourselves, and then overriding the evolutions used for this app. This is now done in a new `reviewboard.upgrade` module, which has pre-upgrade and post-upgrade steps that can be applied by both `manage.py` and `rb-site upgrade`. The pre-upgrade step deletes the migration and evolution history for `oauth2_provider` and modifies the stored signature. That then allows our custom evolutions to apply on top of it. If this is a completely new install, none of this kicks in, but Django Evolution will proceed with the standard evolution process for creating the models for the app, marking those evolutions as applied and setting us up for future evolutions to this app down the road. There's also a fix for the `Profile` model, which lacked evolutions for one of the changes made for 5.0.	28ae86ed0d6fae1cfbce762b25c07d3718acd357

Diff:

Revision 2 (+432 -18)

Show changes

	reviewboard/manage.py
	reviewboard/upgrade.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_default_use_rich_text_boolean_field.py
	reviewboard/admin/custom_evolutions/oauth2_provider/__init__.py
	reviewboard/admin/custom_evolutions/oauth2_provider/move_to_migrations.py
	reviewboard/admin/custom_evolutions/oauth2_provider/upgrade_to_1_6_3.py
	reviewboard/cmdline/rbsite.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to django-3.2 (66509d3)