Add --strict flag #441

philip-hartmann · 2023-05-04T20:05:10Z

This adds the --strict-order flag which ignores all out of order pending migrations. With the flag set dbmate up and dbmate migrate only apply pending migrations which succeed the latest applied migration - in other words, only pending migrations with a higher version number than the latest applied migration will be used. dbmate --strict-order status also only lists in order pending migrations. Rollbacks work as usual.

Refs: #159

gregwebs · 2023-05-05T18:59:54Z

This code change looks good to me. From a UX perspective I find --strict-order a confusing name. Something like --new-only would make more sense to me.

It's also wort noting that my PR #438 would allow you to select a range of migrations, such as the range that doesn't include some older migrations- so a slightly more manual version of this. For me it seems like it would work because this use case wouldn't come up a lot.

philip-hartmann · 2023-05-07T08:36:19Z

@amacneil proposed the name --strict-order for the flag in #159, so I went with it. The flag comes in handy as a permanent flag you would choose to make your migration strategy more strict, to avoid migration conflicts mentioned in #159 and crudecomputer/jrny/issues/17.

I also do not see --strict-order in conflict with #438, as they both tackle different use cases.

gregwebs · 2023-05-07T16:00:12Z

Definitely not in conflict. We don't have to bikeshed about the name- it's fine as is- but just thought I would throw a perspective out there.

amacneil · 2023-05-12T06:55:17Z

My linked comment was 3 years ago, but I think probably just --strict is even better for the flag name? It seems to be in the spirit of "be strict about applying migrations" in order, and fail if you can't".

I think this should be a flag on the verb, rather than a global flag. It only applies to the up/migrate command.

I'll let others do code review @gregwebs @dossy

Limits the ´--strict-order´ flag to the up, migrate and status verb.

gregwebs · 2023-05-13T03:09:07Z

main.go

@@ -164,6 +176,11 @@ func NewApp() *cli.App {
 			Name:  "status",
 			Usage: "List applied and pending migrations",
 			Flags: []cli.Flag{
+				&cli.BoolFlag{


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}

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 👍

gregwebs · 2023-05-13T03:15:38Z

I think there would be some benefit to instead of just ignoring migrations to have another state in the UI to show that migrations were cutoff due to the strict setting. Users may set this as an env variable and then get confused as to why migrations aren't showing up.

That being said, users that set this will probably be more advanced and I don't have an answer as to what exactly to show users (just that there is a strict cutoff, or a different kind of marking instead of applied/unapplied).
As is this makes for a simple implementation and UI improvements could still be made with future changes.

dossy

Code looks good to me.

For the record, I would never expect something called "strict" to ever ignore anything. "--ignore-outdated" may be more verbose but I would feel more confident guessing exactly how the expected behavior of that option would modify execution, vs. "strict" which I would expect would result in a fatal error if there were unapplied migrations or anything else that would not pass a "strict" test.

But, if the consensus is that "strict" is unambiguous and the implemented behavior matches the expectation of others aside from me, then I'm okay with it.

pkg/dbmate/db.go

philip-hartmann · 2023-05-13T07:02:31Z

The more I think about the flag name the more I am bothered by it. @gregwebs and @dossy raise good points about the unambiguousness of the flag, both on the user level and in the code itself too. A more verbose name would definitely not hurt and would make a condition like if db.Strict much more expressive. Also, could there be a case that strict will be useful for something else in the future or is strict in the context of dbmate unambiguous with applying migrations in order?

amacneil · 2023-05-17T18:00:32Z

Actually, I didn't fully comprehend the PR description, but I was expecting this behavior too:

"strict" which I would expect would result in a fatal error if there were unapplied migrations or anything else that would not pass a "strict" test.

Would that be a better solution, rather than simply ignoring out of order migrations? If any migrations have a lower version number than the current "latest" version, then throw an error and exit 1?

Combined with something like #434 the user could then resolve the issue by manually applying migrations.

esceer · 2023-07-17T08:04:02Z

Is this strict feature planned to be merged anytime soon? We are eager to use it once available.

x80486 · 2023-07-25T21:49:45Z

I think it would be great if when this is merged, a fix command could be implemented so the migrations that are borked can be renamed correctly and "align" them automatically.

philip-hartmann · 2023-07-29T15:25:54Z

Would that be a better solution, rather than simply ignoring out of order migrations? If any migrations have a lower version number than the current "latest" version, then throw an error and exit 1?

Sounds reasonable. I've made the appropriate changes. Strict mode throws if a pending migration is not strictly in numerical order with already applied migrations.

Migrations with the same version number are not yet properly checked though. Dbmate only saves the version number of applied migrations and not the full filename. Therefore we cannot properly compare pending migrations with already applied migrations.

dossy · 2023-07-30T17:29:19Z

Migrations with the same version number are not yet properly checked though. Dbmate only saves the version number of applied migrations and not the full filename. Therefore we cannot properly compare pending migrations with already applied migrations.

In my opinion, version numbers should be unique, and two files with the same version number should raise an error and halt.

This change would break backwards-compatibility, unfortunately.

What do others think?

amacneil · 2023-08-01T06:18:55Z

In my opinion, version numbers should be unique, and two files with the same version number should raise an error and halt.
This change would break backwards-compatibility, unfortunately.

I think two files with the same version number should be considered an error always. We could introduce it behind a different flag and enable it by default in the next major version bump. However, I think this is an unrelated problem to the current PR.

main.go

amacneil · 2023-08-01T06:25:01Z

pkg/dbmate/db.go

@@ -441,6 +453,10 @@ func (db *DB) FindMigrations() ([]Migration, error) {
 				migration.Applied = true
 			}

+			if db.Strict && !migration.Applied && migration.Version < latestAppliedMigration {
+				return nil, fmt.Errorf("cannot apply migration `%s` after `%s` in --strict mode, migrations have to be in numerical order", migration.Version, latestAppliedMigration)


similar to my other comment, I think this error message could be a bit more helpful. we can mirror whatever words are selected for the flag description.

Changed the error message to the following:

migration `%s` is out of order with already applied migrations, the version number has to be higher than the applied migration `%s` in --strict mode

amacneil · 2023-08-01T06:25:08Z

I'm 👍 on the current functionality of this PR, after we resolve my comment about the words used to describe the feature and error message.

amacneil · 2023-08-03T23:03:33Z

pkg/dbmate/db.go

@@ -441,6 +453,10 @@ func (db *DB) FindMigrations() ([]Migration, error) {
 				migration.Applied = true
 			}

+			if db.Strict && !migration.Applied && migration.Version < latestAppliedMigration {


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

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.

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.

…tions()`

FindMigrations returns a sorted list so we only need to check the first pending migration.

A return error is not in line with tests.

philip-hartmann · 2023-11-05T18:19:09Z

@amacneil my last changes are from 3 months ago without any feedback/review so far, is there anything else open about this PR?

pkg/dbmate/db.go

dossy

The code looks good to me now, the only thing left is to add a few new test cases to pkg/dbmate/db_test.go:

while strict=false, allows applying migration out of order, the expected behavior before this change was introduced
while strict=true, allows applying migration in order when no previous migrations have been applied yet
while strict=true, allows applying migration in order when at least one previous migration has been applied and the new migration is in the correct order
while strict=true, raises an error when trying to apply a migration out of order

If you need help implementing the tests, let me know and I can help.

philip-hartmann · 2023-11-10T13:26:11Z

@dossy I've added 2 tests which should cover the 4 cases. Can you please check them?

dossy

Code reviewed, tests passing: unless someone objects, I think this is good to merge now.

dossy · 2023-11-10T13:48:13Z

@amacneil If there's something still unresolved that you'd like to see changed in this PR, could you add a note here letting us know? Otherwise, I'd like to clear your change requested bit on this PR and merge it.

Concerns have been addressed, PR is otherwise ready to merge.

Add --strict-order flag

eb36c90

philip-hartmann added 2 commits May 12, 2023 13:07

Limit --strict-order flag to appropriate verbs

68da288

Limits the ´--strict-order´ flag to the up, migrate and status verb.

Rename --strict-flag to --strict

9822a96

gregwebs reviewed May 13, 2023

View reviewed changes

dossy approved these changes May 13, 2023

View reviewed changes

dossy reviewed May 13, 2023

View reviewed changes

pkg/dbmate/db.go Outdated Show resolved Hide resolved

philip-hartmann added 3 commits July 29, 2023 16:58

Throw error on strict numerical order conflict

d278d6a

Identify latest applied migration only in strict mode

dc0e0a1

Change wording of strict mode error message

f912ddf

amacneil changed the title ~~Add --strict-order flag~~ Add --strict flag Aug 1, 2023

amacneil reviewed Aug 1, 2023

View reviewed changes

main.go Outdated Show resolved Hide resolved

amacneil reviewed Aug 1, 2023

View reviewed changes

Update usage text

5b890ff

amacneil previously requested changes Aug 3, 2023

View reviewed changes

philip-hartmann added 3 commits August 11, 2023 10:12

Check pending migrations version in Migrate() instead of `FindMigra…

d9d7ce2

…tions()`

Change wording of strict mode error message

b9e0bdf

Check only pending migrations in Migrate()

d0ded5d

philip-hartmann added 5 commits August 11, 2023 11:01

Check only first pending migration

311dc19

FindMigrations returns a sorted list so we only need to check the first pending migration.

Abort migration if no pending migrations files exist

d20cf09

Abort migration with no error if no pending migrations files exist

cd88906

A return error is not in line with tests.

Derive highest applied version number from migration list

3323fa5

Undo refactor to find applied migrations

d896ce2

philip-hartmann requested review from amacneil, gregwebs and dossy August 11, 2023 10:42

Rutulpatel7077 approved these changes Sep 6, 2023

View reviewed changes

dossy requested changes Nov 6, 2023

View reviewed changes

pkg/dbmate/db.go Outdated Show resolved Hide resolved

Dump schema even if no migrations are applied

4b85b89

dossy requested changes Nov 7, 2023

View reviewed changes

pkg/dbmate/db.go Outdated Show resolved Hide resolved

Fix backwards-incompatible breaking condition

5d95143

dossy requested changes Nov 10, 2023

View reviewed changes

philip-hartmann added 3 commits November 10, 2023 13:39

Add tests for strict mode

c216c93

Fix tests

9949e88

Remove whitespaces

02d2d7b

dossy approved these changes Nov 10, 2023

View reviewed changes

dossy merged commit 1658a46 into amacneil:main Nov 14, 2023

dossy mentioned this pull request Nov 14, 2023

add ability to specify rollback limit/amount and the target version to migrate to #256

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --strict flag #441

Add --strict flag #441

philip-hartmann commented May 4, 2023 •

edited

Loading

gregwebs commented May 5, 2023

philip-hartmann commented May 7, 2023

gregwebs commented May 7, 2023

amacneil commented May 12, 2023

gregwebs May 13, 2023 •

edited

Loading

philip-hartmann May 13, 2023

gregwebs commented May 13, 2023

dossy left a comment •

edited

Loading

philip-hartmann commented May 13, 2023

amacneil commented May 17, 2023

esceer commented Jul 17, 2023

x80486 commented Jul 25, 2023

philip-hartmann commented Jul 29, 2023

dossy commented Jul 30, 2023

amacneil commented Aug 1, 2023

amacneil Aug 1, 2023

philip-hartmann Aug 11, 2023

amacneil commented Aug 1, 2023

amacneil Aug 3, 2023

philip-hartmann Aug 4, 2023

philip-hartmann Aug 11, 2023

philip-hartmann commented Nov 5, 2023

dossy left a comment •

edited

Loading

philip-hartmann commented Nov 10, 2023

dossy left a comment

dossy commented Nov 10, 2023

Add --strict flag #441

Add --strict flag #441

Conversation

philip-hartmann commented May 4, 2023 • edited Loading

gregwebs commented May 5, 2023

philip-hartmann commented May 7, 2023

gregwebs commented May 7, 2023

amacneil commented May 12, 2023

gregwebs May 13, 2023 • edited Loading

Choose a reason for hiding this comment

philip-hartmann May 13, 2023

Choose a reason for hiding this comment

gregwebs commented May 13, 2023

dossy left a comment • edited Loading

Choose a reason for hiding this comment

philip-hartmann commented May 13, 2023

amacneil commented May 17, 2023

esceer commented Jul 17, 2023

x80486 commented Jul 25, 2023

philip-hartmann commented Jul 29, 2023

dossy commented Jul 30, 2023

amacneil commented Aug 1, 2023

amacneil Aug 1, 2023

Choose a reason for hiding this comment

philip-hartmann Aug 11, 2023

Choose a reason for hiding this comment

amacneil commented Aug 1, 2023

amacneil Aug 3, 2023

Choose a reason for hiding this comment

philip-hartmann Aug 4, 2023

Choose a reason for hiding this comment

philip-hartmann Aug 11, 2023

Choose a reason for hiding this comment

philip-hartmann commented Nov 5, 2023

dossy left a comment • edited Loading

Choose a reason for hiding this comment

philip-hartmann commented Nov 10, 2023

dossy left a comment

Choose a reason for hiding this comment

dossy commented Nov 10, 2023

philip-hartmann commented May 4, 2023 •

edited

Loading

gregwebs May 13, 2023 •

edited

Loading

dossy left a comment •

edited

Loading

dossy left a comment •

edited

Loading