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

Add Alembiс for db migrations #562

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Roc25
Copy link
Contributor

@Roc25 Roc25 commented Oct 25, 2024

For #561

Migrations should be created and applied automatically when user opens library

@Roc25 Roc25 marked this pull request as ready for review October 25, 2024 10:47
@Roc25 Roc25 marked this pull request as draft October 25, 2024 10:51
@Roc25
Copy link
Contributor Author

Roc25 commented Oct 25, 2024

Currently, every time the library is opened, a empty migration file is created. And at this moment I don't know how to to avoid creating empty migration files and leave them to be created automatically. Only way that i see is manual addition of migration files by developers (I'm not expert here, so i think it may cause errors)

Need help

@CyanVoxel CyanVoxel added Status: Help Wanted Extra attention is needed TagStudio: Library Relating to the TagStudio library system labels Oct 26, 2024
@yedpodtrzitko
Copy link
Collaborator

see this commit addressing some issues mentioned
yedpodtrzitko@17e2df1

  • migrations/versions/* shouldnt be in .gitignore because we actually need to keep the generated migrations inside repository

  • there's a problem with alembic.ini location. it's outside the tagstudio folder, which assumes the alembic import statements should start with tagstudio. Howver in env.py, you use import starting with src. (subfolder of tagstudio), which implies that alembic is executed from within the tagstudio folder... but that doesnt actually match the content of alembic.ini. So either the imports need to change, or the content (and ideally location) of alembic.ini needs to be inside tagstudio. In the commit above I changed content of alembic.ini.

  • to generate the migrations, there needs to be some database to compare its content with the models to generate the difference for migrations. I added an empty database file into the alembic folder and assigned its path to sqlachemy.url in alembic.ini

  • after that it's possible to generate the initial migration via:

# inside `tagstudio` folder
alembic -c ../alembic.ini revision --autogenerate -m "initial migration"

@Roc25
Copy link
Contributor Author

Roc25 commented Oct 28, 2024

I am very grateful for your help. As far as I understand, if someone changes the models, they will have to create a migration file.

I will remove the automatic creation of migration files and test how everything works now

@Roc25
Copy link
Contributor Author

Roc25 commented Oct 28, 2024

If I understand you correctly, we can also not change imports and not move alembic.ini, but we can add /tagstudio to sys.path

In alembic.ini
prepend_sys_path = . tagstudio

In that way we can use alembic in root dir and imports will be ok. If I missed something let me know

Apply init migration to alembic_current

Remove auto creation of migration file
@Roc25 Roc25 marked this pull request as ready for review October 28, 2024 13:49
@yedpodtrzitko
Copy link
Collaborator

it doesnt work for me. When I try to run TS from your branch and open a library, I end up with this error:

Traceback (most recent call last):
  File "/Users/yed/dev/python/TagStudio/tagstudio/src/qt/widgets/preview_panel.py", line 374, in <lambda>
    return lambda: self.driver.open_library(Path(path))
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/yed/dev/python/TagStudio/tagstudio/src/qt/ts_qt.py", line 1118, in open_library
    open_status = self.lib.open_library(path)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/yed/dev/python/TagStudio/tagstudio/src/core/library/alchemy/library.py", line 170, in open_library
    self.migrate(connection_string.render_as_string(hide_password=False))
  File "/Users/yed/dev/python/TagStudio/tagstudio/src/core/library/alchemy/library.py", line 147, in migrate
    command.stamp(alembic_cfg, "head")
  File "/Users/yed/.pyenv/versions/312-tags/lib/python3.12/site-packages/alembic/command.py", line 665, in stamp
    script = ScriptDirectory.from_config(config)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/yed/.pyenv/versions/312-tags/lib/python3.12/site-packages/alembic/script/base.py", line 170, in from_config
    raise util.CommandError(
alembic.util.exc.CommandError: No 'script_location' key found in configuration.

@Roc25
Copy link
Contributor Author

Roc25 commented Nov 1, 2024

Fixed it .bat and .sh files. We must run the application from tagstudio, not from the root directory, otherwise the built program (.exe) will give an error

Before the fix, if we ran the program via .sh or .bat for Alembic needs a path relative to the root directory, if we were building a program, then we needed a path relative to the tagstudio folder. That's also why I moved the alembic.ini file from the root directory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Help Wanted Extra attention is needed TagStudio: Library Relating to the TagStudio library system
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants