-
Notifications
You must be signed in to change notification settings - Fork 68
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
updated data base migration to current release #2410
Conversation
I'm confused, how is the migration intended to be applied? Shouldn't the migration script be added to mslib/mscolab/migrations/versions or something? |
We added the migration facility not from the beginning of mscolab. The tool checks the existing database and builds from the model of the installation a migration script. It is important that a new database gets a revision id. This is implemented. The mig script has the revision id which it can work on. When we add them to the dir we would have revision ids which are different to an existing productive database.
We use them like in the video, having on the production system the database for the migrations. We use the documentation for hints. Sometimes the creation of the mig script is not all to do. This time the mig script was not able on a database with users to solve correctly:
We can add them under revision control but I think we should document them too. When we start with flask db init on a fresh system, we have no down_revision to rollback. I would have expected that.
|
Doesn't matter.
Yes. This should only happen once and the resulting (possibly fixed up, like in this case) migration script committed to the repository, so that the admin of a mscolab server only needs to run
This should never be the case. If this is the case then either we already skipped adding a migration script to the repository or the production database was messed with outside of what the mscolab application ships with and as a developer I am not interested in supporting this situation. The migration scripts in the repository should form a continuous chain (or actually more like a DAG, if we consider branching) from one database revision to the next, and any state outside of that should be unreachable.
This is by design, once the migration script is committed to the repository (or at least once it is part of a release) it should be considered immutable.
This error looks like a previous migration script wasn't committed to the repository, which is a bug.
That's why the fixed migration script is added to the repository, to avoid having to repeat manual steps.
Documentation should be some variant of "Run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see my previous comments. As-is this doesn't follow the recommendations outlined in the video linked by Reimar. The migration script would need to be added to the repository and all missing migrations between the last one contained in the repository and the current state of the model added as well. Alternatively, the migration script has to be recreated based on the latest migration that is contained in the repository. Or alternatively, we can start the migrations from scratch and ensure all future migration scripts are added properly.
The documentation needs a change. That was missing by the release. Additionally we should try to understand why the migration scripts are not used from our library and how to solve this without breaking other functionality. When we have a solution which works always also on the server we could remove documenting this. The documentation I linked is a good start for deploying from a directory, which we don't. We don't have an app.py in the users home dir. And when creating a wsgi script we have a new start path for the revision scripts which differs from the package. I think we should work on this in a new issue. I assume that there is some refactoring needed. This PR should primary fix the documentation. |
Maybe, but this change is not what is needed. An admin of an mscolab server never has to run What is missing in the release are the migration scripts in mslib/mscolab/migrations.
I don't get what you mean. It is quite a mouthful, but you can get at the migrations with an installed version of mss without a problem, e.g.:
On a server no migration script should ever be created. That should solely happen in development. |
Make an issue for this and provide the solution. This PR is for the documentation we have released. |
OK, will do. This PR contains a documentation change that we shouldn't make, so I vote for just closing it. |
I added an issues about the ideas I have on this #2417 I don't think it is a good idea to have in the docs a migration to 8 script which is wrong compared to what to do for version 9. That is currently the only way a user can get help. It is not that easy to know how to fix the makemigrations failure. |
The proper fix is actually rather easy, it just needs to be done. |
I mean, we could merge this PR until the proper fix is done and no manual migrations will ever be necessary, but that will bring us into a situation in which we will have to document multiple migration paths depending on what people currently have in their production databases and it will be a pain to fix if people fail to apply that migration from v8 to v9 properly. |
the easist solution I add to the PR section that this mig path documentation is deprecated and will be removed with the next major. because of the skyfield-data problem I want to release soon. btw. we will also learn on our servers first what it means to switch to the better path. Others having the same problems I might be a ressource for help. |
OK, let's go with that. The "skyfield-data problem" is nothing that necessarily really requires a release though; since mamba does install time dependency resolution it should already pick up the correct version if it is released on conda-forge. |
Do not expect that users type mamba update on theire existing Installation. Computation errors can be problematic. |
Purpose of PR?:
Fixes #2401
We need to update the documentation