-
Notifications
You must be signed in to change notification settings - Fork 523
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
Sequential migrations #27
Sequential migrations #27
Conversation
24ad0f7
to
ac2d2ba
Compare
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.
How does this solve a following "race condition"?
- First developer merges PR with a newer migration (20170329) and deploys it.
- Second developer merges an older PR with migration (20170215) and deploys it?
The second migration would get lost and wouldn't get executed, since it has lower version number than the already applied migrations in the DB.
Goose compares migration versions as integers against the most recently applied migration version in the DB. It doesn't check if there's any migration "lost in the past". The library is not very complex.
I know that manual migration versioning is very annoying aspect of Goose right now, but at least it's stable and consistent. If you have a conflict (same version of migration) in the codebase, it errors out.
The time-based migration versions wouldn't work with the current Goose implementation. It would require large refactor to make it work
--
btw: I'm just leaving for vacation and won't be available in next 2 weeks. So I'll get back to this PR once I'm back. Feel free to ping me again.
Thanks for understanding.
@@ -173,7 +176,7 @@ language plpgsql; | |||
A [sample Go migration 00002_users_add_email.go file](./example/migrations-go/00002_rename_root.go) looks like: | |||
|
|||
```go | |||
package migrations | |||
package main |
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.
why this change?
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.
This is golang requirement (https://tip.golang.org/pkg/plugin/).
[...] A plugin is a Go main package with exported functions [...]
Without this change, plugin do not compile correctly (resulting binary is not a valid ELF binary). Fortunately, this change does not break goose API.
In order to eliminate migration collisions we have to maintain only sequential migration from the very beginning:
The above procedure wouldn't work if any of the two developers use a date-based migration. To not break backward compatibility, 'create' sub-command is not changed but instead a new 'create-next' is added. Developers should be instructed to use 'create-next' to be safe from migration collisions. Another requirement for a developer is to commit a migration script at the same time database schema is changed so that different migration scripts and actual changes to database do not mix. |
func Create(db *sql.DB, dir, name, migrationType string, sequential bool) error { | ||
var version string | ||
if !sequential { | ||
version = time.Now().Format("20060102150405") |
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 see, what you just explained makes sense. However, the above line confused me. We shouldn't ever suggest version based on a time date.
ac2d2ba
to
2d3ada4
Compare
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.
Seems like there are still left overs from the other PR #26. Can you please remove them?
Let's keep this branch strictly to enforce sequential numbering, as a suggested practice.
There is confusing line at create.go:13, though:
version = time.Now().Format("20060102150405")
Is this a mistake? It shouldn't suggests timestamp but 0000
or 0001
as initial first migration version.
Superseded by #50. Thanks for this PR anyway, you brought up a great point about goose versioning! |
This PR solved #25