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 --strict flag #441

Merged
merged 20 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
eb36c90
Add --strict-order flag
philip-hartmann May 4, 2023
68da288
Limit --strict-order flag to appropriate verbs
philip-hartmann May 12, 2023
9822a96
Rename --strict-flag to --strict
philip-hartmann May 12, 2023
d278d6a
Throw error on strict numerical order conflict
philip-hartmann Jul 29, 2023
dc0e0a1
Identify latest applied migration only in strict mode
philip-hartmann Jul 29, 2023
f912ddf
Change wording of strict mode error message
philip-hartmann Jul 29, 2023
5b890ff
Update usage text
amacneil Aug 3, 2023
d9d7ce2
Check pending migrations version in `Migrate()` instead of `FindMigra…
philip-hartmann Aug 11, 2023
b9e0bdf
Change wording of strict mode error message
philip-hartmann Aug 11, 2023
d0ded5d
Check only pending migrations in `Migrate()`
philip-hartmann Aug 11, 2023
311dc19
Check only first pending migration
philip-hartmann Aug 11, 2023
d20cf09
Abort migration if no pending migrations files exist
philip-hartmann Aug 11, 2023
cd88906
Abort migration with no error if no pending migrations files exist
philip-hartmann Aug 11, 2023
3323fa5
Derive highest applied version number from migration list
philip-hartmann Aug 11, 2023
d896ce2
Undo refactor to find applied migrations
philip-hartmann Aug 11, 2023
4b85b89
Dump schema even if no migrations are applied
philip-hartmann Nov 7, 2023
5d95143
Fix backwards-incompatible breaking condition
philip-hartmann Nov 8, 2023
c216c93
Add tests for strict mode
philip-hartmann Nov 10, 2023
9949e88
Fix tests
philip-hartmann Nov 10, 2023
02d2d7b
Remove whitespaces
philip-hartmann Nov 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ The following options are available with all commands. You must use command line
- `--env, -e "DATABASE_URL"` - specify an environment variable to read the database connection URL from.
- `--migrations-dir, -d "./db/migrations"` - where to keep the migration files. _(env: `DBMATE_MIGRATIONS_DIR`)_
- `--migrations-table "schema_migrations"` - database table to record migrations in. _(env: `DBMATE_MIGRATIONS_TABLE`)_
- `--strict` - ignore out of order pending migrations. _(env: `DBMATE_STRICT`)_
- `--schema-file, -s "./db/schema.sql"` - a path to keep the schema.sql file. _(env: `DBMATE_SCHEMA_FILE`)_
- `--no-dump-schema` - don't auto-update the schema.sql file on migrate/rollback _(env: `DBMATE_NO_DUMP_SCHEMA`)_
- `--wait` - wait for the db to become available before executing the subsequent command _(env: `DBMATE_WAIT`)_
Expand Down
18 changes: 18 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ func NewApp() *cli.App {
Name: "up",
Usage: "Create database (if necessary) and migrate to the latest version",
Flags: []cli.Flag{
&cli.BoolFlag{
Name: "strict",
EnvVars: []string{"DBMATE_STRICT"},
Usage: "ignore out of order pending migrations",
},
&cli.BoolFlag{
Name: "verbose",
Aliases: []string{"v"},
Expand All @@ -109,6 +114,7 @@ func NewApp() *cli.App {
},
},
Action: action(func(db *dbmate.DB, c *cli.Context) error {
db.Strict = c.Bool("strict")
db.Verbose = c.Bool("verbose")
return db.CreateAndMigrate()
}),
Expand All @@ -131,6 +137,11 @@ func NewApp() *cli.App {
Name: "migrate",
Usage: "Migrate to the latest version",
Flags: []cli.Flag{
&cli.BoolFlag{
Name: "strict",
EnvVars: []string{"DBMATE_STRICT"},
Usage: "ignore out of order pending migrations",
},
&cli.BoolFlag{
Name: "verbose",
Aliases: []string{"v"},
Expand All @@ -139,6 +150,7 @@ func NewApp() *cli.App {
},
},
Action: action(func(db *dbmate.DB, c *cli.Context) error {
db.Strict = c.Bool("strict")
db.Verbose = c.Bool("verbose")
return db.Migrate()
}),
Expand All @@ -164,6 +176,11 @@ func NewApp() *cli.App {
Name: "status",
Usage: "List applied and pending migrations",
Flags: []cli.Flag{
&cli.BoolFlag{
Copy link

@gregwebs gregwebs May 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a precedent for duplication with the verbose flag, but it seems like it would be better to define the flag just once

strictFlag := cli.BoolFlag{...}
...
flags: []cli.Flag{&strictFlag}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could really tidy up the duplicate flag usage. I was already a little annoyed writing that part but I tried to do it similar to the verbose flag. I will change it 👍

Name: "strict",
EnvVars: []string{"DBMATE_STRICT"},
Usage: "ignore out of order pending migrations",
},
&cli.BoolFlag{
Name: "exit-code",
Usage: "return 1 if there are pending migrations",
Expand All @@ -174,6 +191,7 @@ func NewApp() *cli.App {
},
},
Action: action(func(db *dbmate.DB, c *cli.Context) error {
db.Strict = c.Bool("strict")
setExitCode := c.Bool("exit-code")
quiet := c.Bool("quiet")
if quiet {
Expand Down
14 changes: 14 additions & 0 deletions pkg/dbmate/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ type DB struct {
MigrationsTableName string
// SchemaFile specifies the location for schema.sql file
SchemaFile string
// Ignore out of order pending migrations
Strict bool
// Verbose prints the result of each statement execution
Verbose bool
// WaitBefore will wait for database to become available before running any actions
Expand All @@ -75,6 +77,7 @@ func New(databaseURL *url.URL) *DB {
MigrationsDir: []string{"./db/migrations"},
MigrationsTableName: "schema_migrations",
SchemaFile: "./db/schema.sql",
Strict: false,
Verbose: false,
WaitBefore: false,
WaitInterval: time.Second,
Expand Down Expand Up @@ -412,6 +415,13 @@ func (db *DB) FindMigrations() ([]Migration, error) {
}
}

var latestAppliedMigration string
for migrationVersion := range appliedMigrations {
if migrationVersion > latestAppliedMigration {
latestAppliedMigration = migrationVersion
}
dossy marked this conversation as resolved.
Show resolved Hide resolved
}

migrations := []Migration{}
for _, dir := range db.MigrationsDir {
// find filesystem migrations
Expand Down Expand Up @@ -441,6 +451,10 @@ func (db *DB) FindMigrations() ([]Migration, error) {
migration.Applied = true
}

if db.Strict && !migration.Applied && migration.Version < latestAppliedMigration {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right place for this check to live. FindMigrations() should not fail regardless of what migrations have or haven't been applied.

The check should be implemented in the Migrate() function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without an additional db query the check needs access to a list of applied migrations to find the applied migration with the highest version number. FindMigrations() already queries those to check if a migration is already applied. There are several ways to do the check outside of FindMigrations() but I did not want to make too many unrelated changes which might be in conflict with other PRs, e.g. changing the return values of FindMigrations() would be in conflict with PR #438.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the strict version check in Migrate() and applied some small changes. FindMigrations() already returns a sorted list of migrations so we only need to check the first pendig migration with the highest version number from all applied migrations.

continue
}

migrations = append(migrations, migration)
}
}
Expand Down