Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken and incomplete mscolab migration scripts and flask-migrate setup #2416

Open
matrss opened this issue Jun 27, 2024 · 11 comments · May be fixed by #2432
Open

Fix broken and incomplete mscolab migration scripts and flask-migrate setup #2416

matrss opened this issue Jun 27, 2024 · 11 comments · May be fixed by #2432
Assignees
Milestone

Comments

@matrss
Copy link
Collaborator

matrss commented Jun 27, 2024

The migration scripts in mslib/mscolab/migrations are incomplete and broken, as in the ones that are there can not be applied.

What should be done IMO:

  1. Add tests that check the consistency and up-to-date-ness of migration scripts (flask_migrate.check should be a good starting point)
  2. Fix migration scripts so that flask db upgrade works
  3. Get rid of any manual database initialization that mscolab also does (e.g. db --init), rely on flask-migrate instead
  4. Run database migrations automatically on startup of mscolab
  5. Remove any documentation that suggests manually doing migrations, e.g. with flask db migrate

This came up in #2410.

@matrss
Copy link
Collaborator Author

matrss commented Jun 27, 2024

There will necessarily be a transition period where production databases don't have their alembic_version set, so some manual steps will be required (mainly flask db stamp to the database revision that matches the actual database in production). From which versions of mscolab do we want to support automatic migrations? I am considering recreating an initial migration from the latest v8 release and then doing additional migration scripts until up-to-date, which would mean that production systems are assumed to be at least on the latest v8 release.

@matrss matrss changed the title Fix broken and incomplete mscolab migration scripts Fix broken and incomplete mscolab migration scripts and flask-migrate setup Jun 27, 2024
@ReimarBauer
Copy link
Member

I merge with #2417

@ReimarBauer
Copy link
Member

Currently we have a not standard solution for doing migrations on a server. The knowledge was gained when the documentation was updated #2410

The solution should become similiar to https://docs.djangoproject.com/en/5.0/topics/migrations/

On django the managment command is named manage.py on our mscolab server it is mscolab.py

We should implement similiar options as django has. Because on flask's migration I had several times that I needed to modify the created migrations script, we maybe need to figure too why that is needed.

mscolab db --migrate
should apply all needed migration scripts to an existing database

mscolab db --makemigrations
creates the migration script, tells verify working, commit a working state

mscolab db --init
as it does now, we have to discuss if this should have the option to rollback to older revisions of the dbase model

mscolab db --reset
as it does now, see --init

mscolab db --reset
as it does now, see --init

mscolab db --sqlmigrate
displays the SQL statements for a migration.

mscolab db --showmigrations
lists a project’s migrations and their status

mscolab db --start
should tell on start there are migrations not done, and we would need an option to apply automatically.

There are more db commands currently maybe they should be moved to different category as db or cli scripts.

This also means we have to take care/applying migrations during development.

@ReimarBauer
Copy link
Member

There will necessarily be a transition period where production databases don't have their alembic_version set, so some manual steps will be required (mainly flask db stamp to the database revision that matches the actual database in production). From which versions of mscolab do we want to support automatic migrations? I am considering recreating an initial migration from the latest v8 release and then doing additional migration scripts until up-to-date, which would mean that production systems are assumed to be at least on the latest v8 release.

We need at lease one before to start from and rollback. 8 is ok for me

@matrss
Copy link
Collaborator Author

matrss commented Jun 27, 2024

We should implement similiar options as django has. Because on flask's migration I had several times that I needed to modify the created migrations script, we maybe need to figure too why that is needed.

There is nothing to figure out, this is by design. Alembic can not always figure out the correct migration path, it is expected that the generated migration script needs to be adapted in some way and the result then committed to the repository. This is the standard mode of operation for flask-migrate.

mscolab db --migrate
should apply all needed migration scripts to an existing database

This should also happen automatically on startup of mscolab.

mscolab db --makemigrations
creates the migration script, tells verify working, commit a working state

Since this should never be done outside of development I don't think we should expose an option on mscolab for that. Instead, we can document the correct flask db migrate invocation for development purposes.

mscolab db --init
as it does now, we have to discuss if this should have the option to rollback to older revisions of the dbase model

This could be merged with --migrate, since a migrate (or the underlying flask db upgrade, then) will create the database from scratch if it doesn't exist yet.

This also means we have to take care/applying migrations during development.

flask db check or rather flask_migrate.check as a pytest test could help with that, detecting if there are any changes in the model that would generate a new migration and failing a test if there are. This would ensure that we get notified when there are any model changes that would require a migration to be added.

@ReimarBauer
Copy link
Member

There is nothing to figure out, this is by design. Alembic can not always figure out the correct migration path, it is expected that the generated migration script needs to be adapted in some away and the result then committed to the repository. This is the standard mode of operation for flask-migrate.

I should have detailed that we use sqlite for development and postgres on the server. One of the earlier updates scripts had crashed on the postgres dbase and I needed to fix it by a sed on a dump.

@ReimarBauer
Copy link
Member

flask db check or rather flask_migrate.check as a pytest test could help with that, detecting if there are any changes in the model that would generate a new migration and failing a test if there are. This would ensure that we get notified when there are any model changes that would require a migration to be added.

Whatever it uses internally I don't want to change the mscolab command to flask as a command.

@ReimarBauer
Copy link
Member

ReimarBauer commented Jun 27, 2024

Since this should never be done outside of development I don't think we should expose an option on mscolab for that. Instead, we can document the correct flask db migrate invocation for development purposes.

ok, for developer it is ok to use flask. This also makes it better to define what's for the users

@matrss
Copy link
Collaborator Author

matrss commented Jun 27, 2024

I should have detailed that we use sqlite for development and postgres on the server. One of the earlier updates scripts had crashed on the postgres dbase and I needed to fix it by a sed on a dump.

Yes, we also need CI test runs against other databases if we want to support them. This kind of incompatibility is the reason why I don't really like ORMs and trying to abstract over multiple SQL databases, it is often just not possible...

@ReimarBauer
Copy link
Member

ReimarBauer commented Jun 27, 2024

flask db check or rather flask_migrate.check as a pytest test could help with that, detecting if there are any changes in the model that would generate a new migration and failing a test if there are. This would ensure that we get notified when there are any model changes that would require a migration to be added.

Yes,
for developers we usually say don't migrate start from fresh. We have to think about if we don't do that, always migrate.
Otherwise we can missout some problems others can have. This means also for the one doing a model change provide the migscript, e.g. gsoc in early state.
I have currently no idea how parallel changes are done on PR for example if we can later simplify 3 migration revisions to 1, e.g. A rollback to a state before first change then squash without migration scripts, then create the needed 1 mig script.
For a future release it would be good to have only the final migration script.

@matrss
Copy link
Collaborator Author

matrss commented Jun 27, 2024

for developers we usually say don't migrate start from fresh. We have to think about if we don't do that, always migrate.

Well, if we properly use flask-migrate there is no difference: recreating from scratch is the same as applying all migrations one after another. The only difference might be the existing data in the database.

But we should have automated tests for all possible upgrade paths and also all possible rollbacks, in my opinion.

I have currently no idea how parallel changes are done on PR for example if we can later simplify 3 migration revisions to 1, e.g. A rollback to a state before first change then squash without migration scripts, then create the needed 1 mig script.
For a future release it would be good to have only the final migration script.

Three options:

  1. Use flask db merge to create a merge migration that combines parallel changes to the database.
  2. Always rebase features branches before merging, including rebasing migration scripts. This would result in a linear history with no parallel changes to the database.
  3. Collect all changes to the database in one migration script until the next release, start a new migration afterwards. This is the least fine-grained option that is viable, in my opinion.

@matrss matrss self-assigned this Jul 8, 2024
@matrss matrss linked a pull request Jul 8, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants