-
Notifications
You must be signed in to change notification settings - Fork 0
[RHM-1969] Gloat should support rollback of migrations. #1
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
Conversation
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.
We use https://pkg.go.dev/github.com/stretchr/testify/assert for assertions, let's drop gsamokovarov/assert
cmd/gloat/main.go
Outdated
return nil | ||
} | ||
|
||
func toCmd(args arguments) error { |
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.
By convention the name toCmd
looks like it should convert the arguments to a Cmd
, can we improve the naming a bit?
migration.go
Outdated
Path: path, | ||
Version: version, | ||
Options: options, | ||
AppliedAt: time.Now(), |
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.
Let's just in case, use time.Now().UTC()
for consistency across platforms?
migration.go
Outdated
@@ -13,6 +14,7 @@ import ( | |||
var ( | |||
now = time.Now().UTC() |
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.
Let's delete this variable it's only used in a single function and is initialized every time you load the package.
migration.go
Outdated
func AppliedAfter(store Source, source Source, version int64) (Migrations, error) { | ||
var appliedAfter Migrations | ||
appliedMigrations, err := store.Collect() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
found := false | ||
for _, migration := range appliedMigrations { | ||
if migration.Version == version { | ||
found = true | ||
break | ||
} | ||
appliedAfter = append(appliedAfter, migration) | ||
} |
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.
Do we sort here? If not, we implicitly rely on the db's internals and that's not guaranteed to be stable - you might append migrations that were applied before the one you're looking for
migration.go
Outdated
excepted = append(excepted, migration) | ||
} | ||
} | ||
|
||
return | ||
} | ||
|
||
// Intersect selects migrations that does exist in the current ones. | ||
func (m Migrations) Intersect(migrations Migrations) (intersect Migrations) { |
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 variables m
, migrations
and intersect
aren't super clear here, can we rename migrations
to others
or intersectWith
and intersect
with result
?
Side note, using named returns isn't a good practice unless necessary.
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.
Looks good
@borisbsv can you check this PR again. |
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.
Merge on :)
For rhyme to be able to rollback to a prior version our migrations library was forked and additional features were implemented.
There are two possible ways for reverting migrations between versions:
migrate to <version>
can be used. In that case migrations applied after<version>
will be reverted.migrate latest
command.applied_at
field is being added to the database.migrate keep <list,of,versions,to,keep>
can be used. In that case, all migrations listed (in the comma separated list) will be kept, the rest that are applied will be reverted.migrate present
command.applied_at
field is being added to the database.To deploy this feature, the changes of the
schema_migrations
table should be executed manually:https://coursera.atlassian.net/browse/RHM-1969
TODO: