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

Allow using Session instead of Engine #56

Merged
merged 4 commits into from
Oct 26, 2023
Merged

Allow using Session instead of Engine #56

merged 4 commits into from
Oct 26, 2023

Conversation

qwerty287
Copy link
Contributor

This is yet another change adapted from Woodpecker's migrator:
Instead of using xorm.Engine as arguments for the migration functions, use xorm.Session. The reason for this is that you can rollback sessions if the migration fails, so nothing should be changed.
This change is breaking though, you'd need to change every migration func. Thus, feel free to close this if you don't like it.

@techknowlogick
Copy link
Owner

Thanks for your PRs @qwerty287

@kolaente as this affects you, do you have any thoughts about this change?

I am a bit hesitant with this change as is due to it removing flexibility, as with Gitea we have migrations that require multiple sessions (weird db things that you can't rename a table and update columns in SQLite is one example). Would perhaps instead of this change, instead a wrapper function could be created, so if you wanted to have the nicety of using sessions you could but otherwise you'd have access to the engine? I know go is rather strict with typing so I'm not entirely sure of the viability of this suggestion.

@kolaente
Copy link
Contributor

I wouldn't make sessions the default, for the reasons you noted. Vikunja has 87 migrations, Gitea many more.

An option could be to add another method MigrateWithSession or similar, for cases where you know using sessions is possible and not a problem.

@qwerty287
Copy link
Contributor Author

I can also change this to a separate function so both can be used, if this is fine?

@qwerty287 qwerty287 changed the title Breaking: Use Session instead of Engine Allow using Session instead of Engine Oct 26, 2023
@qwerty287
Copy link
Contributor Author

@techknowlogick @kolaente I changed to support both methods in 5ffddcb, using a separate new field MigrateSession/RollbackSession (if both would be set engine version is preferred).

@techknowlogick
Copy link
Owner

@kolaente much more succinctly put than my rambling paragraph. Thanks :)
@qwerty287 gracias 🙏 I'll merge and cut a release. If there is anything I can do to help you/WP lmk. In case you need an example of xormigrate used in practice, here is a good example: https://kolaente.dev/vikunja/api/src/branch/main/pkg/migration

@techknowlogick techknowlogick merged commit 4ba8b09 into techknowlogick:main Oct 26, 2023
2 checks passed
@qwerty287 qwerty287 deleted the session branch October 27, 2023 09:32
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 this pull request may close these issues.

3 participants