Skip to content

[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

Merged
merged 15 commits into from
Mar 30, 2022

Conversation

ndyakov
Copy link

@ndyakov ndyakov commented Mar 10, 2022

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:

  1. If both version are on the same base migrate to <version> can be used. In that case migrations applied after <version> will be reverted.
  • to get the latest migration that is part of a given version, there is migrate latest command.
  • for correct ordering of migrations applied_at field is being added to the database.
  1. If the two versions are not part of the same node and their migrations may be applied out of order, the 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.
  • to generate the list of migrations that are part of a given version, there is migrate present command.
  • for correct ordering of migrations applied_at field is being added to the database.

To deploy this feature, the changes of the schema_migrations table should be executed manually:

ALTER TABLE schema_migrations 
ADD COLUMN applied_at timestamp without time zone default (now() at time zone 'utc');

CREATE INDEX IF NOT EXISTS schema_migrations_applied_at 
ON schema_migrations (applied_at);

https://coursera.atlassian.net/browse/RHM-1969

TODO:

  • tests
  • update help
  • write deployment steps

@ndyakov ndyakov requested review from Hepri, itsankoff and borisbsv March 10, 2022 07:13
Copy link

@borisbsv borisbsv left a 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

return nil
}

func toCmd(args arguments) error {

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(),

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()

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
Comment on lines 206 to 220
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)
}

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) {

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.

Copy link

@Hepri Hepri left a comment

Choose a reason for hiding this comment

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

Looks good

@ndyakov
Copy link
Author

ndyakov commented Mar 18, 2022

@borisbsv can you check this PR again.

Copy link

@borisbsv borisbsv left a comment

Choose a reason for hiding this comment

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

Merge on :)

@ndyakov ndyakov merged commit fad15ab into master Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants