[cli] Improve migration creation #312

13k · 2019-12-04T08:59:50Z

This PR adds validation to migration versions when creating migrations and improves/refactors creation in general. As a "side-effect", it fixes #238.

Rationale

migrate create doesn't validate migration versions when creating files. This causes issues when creating files that results in same-version migrations. It's easy to replicate this behavior:

# created sequentially too fast (unix format)

$ for name in same_second_one same_second_two; do
$   migrate create -dir migs -ext sql -format "unix" "$name"
$ done
$ ls -1 migs
1575445236_same_second_one.down.sql
1575445236_same_second_one.up.sql
1575445236_same_second_two.down.sql
1575445236_same_second_two.up.sql

# low precision time format (stupid, but valid format)

$ migrate create -dir migs -ext sql -format "20060102" same_day_one
$ migrate create -dir migs -ext sql -format "20060102" same_day_two
$ ls -1 migs
20191204_same_day_one.down.sql
20191204_same_day_one.up.sql
20191204_same_day_two.down.sql
20191204_same_day_two.up.sql

I accidentally hit the first case when converting an already existing database schema into migrations, using a very similar loop, iterating over table names, creating migrations and redirecting the output of pg_dump -st $table into each "up" migration. It didn't occur to me that versions would be colliding. When I tried to run migrate up, a very obscure "unable to parse file" error popped up. Reading the source code explained it.

Since it's much harder to refactor the whole source package to show useful error messages (Migrations.Append() isn't supposed to return error with useful messages instead of bool?), I went for the simpler "warn the kids that the stove is hot" solution.

Solution & Improvements

Validate migration version on creation, to avoid creation of duplicated versions

A refactoring of the migration creation code first generates the version component and then checks if any files with that version already exists on disk.

Here's the corresponding outputs to the commands above, using the fixed version:

$ go build -o /tmp/migrate ./cmd/migrate
$ cd "$(mktemp -d)"

$ /tmp/migrate create -dir migs -ext sql -format "20060102" same_day_one
migs/20191204_same_day_one.up.sql
migs/20191204_same_day_one.down.sql

$ /tmp/migrate create -dir migs -ext sql -format "20060102" same_day_two
error: duplicate migration version: 20191204

$ ls -1 migs
20191204_same_day_one.down.sql
20191204_same_day_one.up.sql

Use os.OpenFile with O_CREATE|O_EXCL to create files to avoid file collisions

This would be very difficult to cause, but I can imagine some bogus uses of parallel + migrate that would case this to happen (in cases someone want to generate hundreds of migrations faster, but the script contains bugs, for example).

Use filepath functions to manipulate paths, making cleanPath() not necessary

I see that in the discussion of #238 and the proposed fix for it in 9f6c7e5 and #250, there's a lot of string manipulation gymnastics to handle cross-platform path manipulation. filepath is the cross-platform solution for this and all its functions should be used when doing real, on disk, OS specific, path manipulation.

So this PR properly fixes #238 and supersedes #250.

Prints generated filenames

This can be debatable, but popular migration generators (active record, django, etc) print the filenames out, so the developer knows which file was created. It's also convenient to simply copy-paste the name of the file to open it for editing.

Tests

The PR updates the migration creation tests, but I'm not able to run the whole suite because I'm on Fedora 31 which ships the kernel with cgroupsv2 enabled and docker doesn't work with it (as of now).

go test ./internal/cli/... does work though.

And I quickly tested the built CLI on a Windows VM: https://gist.github.com/13k/1483f0c377991252b486fd604fd5acee

* Validates migration version on creation, to avoid creation of duplicated versions * Uses `os.OpenFile` with `O_CREATE|O_EXCL` to create files to avoid file collisions * Uses `filepath.Join` to concatenate paths, making `cleanPath()` not necessary * Prints generated filenames * Fixes golang-migrate#238 * Supersedes golang-migrate#250

coveralls · 2019-12-04T09:24:58Z

Pull Request Test Coverage Report for Build 570

63 of 75 (84.0%) changed or added relevant lines in 2 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.8%) to 51.911%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/cli/main.go	0	3	0.0%
internal/cli/commands.go	63	72	87.5%

Files with Coverage Reduction	New Missed Lines	%
internal/cli/commands.go	2	60.59%

Totals
Change from base Build 565:	0.8%
Covered Lines:	4754
Relevant Lines:	9158

💛 - Coveralls

mknycha

Looks good to me

mknycha · 2019-12-06T07:03:12Z

internal/cli/main.go


-		createCmd(*dirPtr, startTime, *formatPtr, name, *extPtr, seq, seqDigits)
+		if err := createCmd(*dirPtr, startTime, *formatPtr, name, *extPtr, seq, seqDigits, true); err != nil {


Consider putting this true in a named variable, so that it's understandable without checking method definition.

None of the other *Cmd() methods return an error. The new method signature where an error is returned is cleaner, it's more important to be consistent. Either use log.fatalErr() in createCmd() or update the other *Cmd() methods to also return an error.

mknycha · 2019-12-06T07:09:33Z

internal/cli/commands.go

-	padding := seqDigits - len(nextSeqStr)
-	if padding > 0 {
-		nextSeqStr = strings.Repeat("0", padding) + nextSeqStr
+	version := fmt.Sprintf("%0[2]*[1]d", nextSeq, seqDigits)


That's pretty neat

+1 to not implementing our own padding logic!

scottagarman · 2019-12-22T00:11:23Z

I currently can't create migrations on windows, hoping this can resolve that issue.

dhui

Thanks for the PR and the great writeup describing your changes!

Please see the feedback below.

dhui · 2019-12-25T22:25:58Z

internal/cli/main.go


-		createCmd(*dirPtr, startTime, *formatPtr, name, *extPtr, seq, seqDigits)
+		if err := createCmd(*dirPtr, startTime, *formatPtr, name, *extPtr, seq, seqDigits, true); err != nil {


None of the other *Cmd() methods return an error. The new method signature where an error is returned is cleaner, it's more important to be consistent. Either use log.fatalErr() in createCmd() or update the other *Cmd() methods to also return an error.

dhui · 2019-12-25T22:26:50Z

internal/cli/commands.go

-	padding := seqDigits - len(nextSeqStr)
-	if padding > 0 {
-		nextSeqStr = strings.Repeat("0", padding) + nextSeqStr
+	version := fmt.Sprintf("%0[2]*[1]d", nextSeq, seqDigits)


+1 to not implementing our own padding logic!

dhui · 2019-12-25T23:44:52Z

internal/cli/commands_test.go

 	}{
-		{dir: "", expectedCleanDir: ""},


Using filepath to build paths should make cleanDir() obsolete. Can you verify that the behavior for these test cases is the same?

internal/cli/commands_test.go

dhui · 2020-01-01T23:07:02Z

internal/cli/commands.go

-		log.fatalErr(err)
+func createFile(filename string) error {
+	// create exclusive (fails if file already exists)
+	f, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)


Add a comment that os.Create() specifies 0666 as the FileMode, so we're doing the same

r3code · 2020-02-26T13:59:07Z

Is there any chance for this PR to be merged soon?

r3code · 2020-03-02T09:51:55Z

@13k I provided some test fixes for OS Windows 13k#1

dhui · 2020-03-02T10:56:33Z

@r3code It might be quicker to create a new PR that references this one and supersedes it. Make your PR still has the original commits from this PR.

r3code · 2020-03-04T07:36:57Z

@r3code It might be quicker to create a new PR that references this one and supersedes it. Make your PR still has the original commits from this PR.

I created a PR #352

dhui · 2020-03-11T16:51:47Z

Superseded by #352

Instead of returning a file not found error when no more changes are available, return a no change message. Fixes golang-migrate#35 Fixes golang-migrate#312

CLI: change createCmd to return error and accept print parameter

7fa10e1

13k force-pushed the cli-create-validate-version branch from 6377852 to 7fa10e1 Compare December 5, 2019 02:05

mknycha reviewed Dec 6, 2019

View reviewed changes

dhui requested changes Jan 1, 2020

View reviewed changes

r3code mentioned this pull request Mar 2, 2020

Cli create validate version 13k/migrate#1

Open

r3code mentioned this pull request Mar 4, 2020

Improved migration creation + Windows fix #352

Merged

dhui closed this Mar 11, 2020

wregis mentioned this pull request Apr 20, 2020

When reaching end of migrations treat as no change #378

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cli] Improve migration creation #312

[cli] Improve migration creation #312

13k commented Dec 4, 2019 •

edited

Loading

coveralls commented Dec 4, 2019 •

edited

Loading

mknycha left a comment

mknycha Dec 6, 2019

dhui Dec 25, 2019

mknycha Dec 6, 2019

dhui Dec 25, 2019

scottagarman commented Dec 22, 2019

dhui left a comment

dhui Dec 25, 2019

dhui Dec 25, 2019

dhui Dec 25, 2019

dhui Jan 1, 2020

r3code commented Feb 26, 2020

r3code commented Mar 2, 2020 •

edited

Loading

dhui commented Mar 2, 2020

r3code commented Mar 4, 2020

dhui commented Mar 11, 2020


		createCmd(dirPtr, startTime, formatPtr, name, *extPtr, seq, seqDigits)
		if err := createCmd(dirPtr, startTime, formatPtr, name, *extPtr, seq, seqDigits, true); err != nil {

[cli] Improve migration creation #312

[cli] Improve migration creation #312

Conversation

13k commented Dec 4, 2019 • edited Loading

Rationale

Solution & Improvements

Tests

coveralls commented Dec 4, 2019 • edited Loading

Pull Request Test Coverage Report for Build 570

💛 - Coveralls

mknycha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottagarman commented Dec 22, 2019

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r3code commented Feb 26, 2020

r3code commented Mar 2, 2020 • edited Loading

dhui commented Mar 2, 2020

r3code commented Mar 4, 2020

dhui commented Mar 11, 2020

13k commented Dec 4, 2019 •

edited

Loading

coveralls commented Dec 4, 2019 •

edited

Loading

r3code commented Mar 2, 2020 •

edited

Loading