-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add db migrations #217
Add db migrations #217
Conversation
c35b317
to
f41a3c6
Compare
database/migrate.go
Outdated
runner.logger = &noopLogger{} | ||
} | ||
|
||
mgr, err := migrate.NewWithDatabaseInstance( |
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 can simplify this a bit, and use migrate.New("file://...", "connection string")
instead of initializing db connection with sql.Open and then passing it no NewWithDatabaseInstance.
database/migrate.go
Outdated
if runner.filesDir == "" { | ||
runner.filesDir = defaultFilesDir | ||
} | ||
|
||
if runner.logger == nil { | ||
runner.logger = &noopLogger{} | ||
} |
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 not setting these fields when initializing MigrationRunner? i.e.
runner := &MigrationRunner{
filesDir: defaultFilesDir,
logger: &noopLogger{},
}
database/migration_runner_env.go
Outdated
|
||
// Constants representing environment variable keys for migration configuration. | ||
const ( | ||
envKeyPrefix = "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.
I think it's better to have duplicated prefixes in the constants below ("MIGRATION_DSN", "MIGRATION_OPERATION"), rather than having a method to concatenate them. It will be much more readable, and easier to search where an env var is used, i.e. when you see an env definition in a deployment config, and would like to know in which package it's used. If env vars defined as a full name, you can just search across all repos and find it.
return fmt.Errorf("missing env: %s", envKey(envKeyOp)) | ||
} | ||
|
||
forceVersion, err := readForceVersion() |
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.
[minor] this is required only for "force" operation, in other cases it's just ignored.
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.
yes and error will occur here only when related env var is set
e40c83d
to
7fabe0a
Compare
@tw-dmy thanks for the comments, all resolved |
Library for migrations code from this pr https://github.com/trustwallet/backend/pull/2331
Additionally added method which reads migration params from env vars and runs migrations, so it can be used in services like