-
Notifications
You must be signed in to change notification settings - Fork 170
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
Enhance Database.Beam.Migrate.Simple #421
base: master
Are you sure you want to change the base?
Enhance Database.Beam.Migrate.Simple #421
Conversation
Hello there... sorry for the long delay. I think this is a good change, but I worry that |
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.
@tathougies Unless I'm missing something, it looks like autoMigrate
will still fail on an unsafe migration.
@@ -211,18 +215,54 @@ autoMigrate :: (Database be db, Fail.MonadFail m) | |||
=> BeamMigrationBackend be m | |||
-> CheckedDatabaseSettings be db | |||
-> m () | |||
autoMigrate BeamMigrationBackend { backendActionProvider = actions | |||
, backendGetDbConstraints = getCs } | |||
autoMigrate b@(BeamMigrationBackend {}) |
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.
I don't think this needs @(BeamMigrationBackend {})
?
Candidates {} -> Fail.fail "planMigration: Could not determine migration" | ||
Solved cmds -> return cmds | ||
|
||
describeMigrationPlan :: Sql92DisplaySyntax (BeamSqlBackendSyntax be) |
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.
The type signature and implementation of this are both pretty specific to the use in autoMigrateVerbose
. I think we should not export it.
-- Check if any of the commands are irreversible | ||
case foldMap migrationCommandDataLossPossible cmds of | ||
MigrationKeepsData -> mapM_ (runNoReturn . migrationCommand) cmds | ||
_ -> Fail.fail "executeMigrationPlan: Not performing automatic migration due to data loss" |
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.
nit: this should match MigrationKeepsData
explicitly.
From obsidiansystems@5f73965