-
Notifications
You must be signed in to change notification settings - Fork 499
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
keystore: add migrate command to handle migrations #1253
Conversation
This PR introduces `keystored migrate up/down` command to handle migrations. It also defines an initial migration for the encrypted_keys table.
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.
Logic looks great! Just a few smaller changes, mostly around formatting.
@@ -19,6 +19,28 @@ cd github.com/stellar/go/exp/services/keystore | |||
go install ./cmd/keystored | |||
``` | |||
|
|||
Set up `keytore` Postgres database locally: |
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.
Nit: keystore
case "up": | ||
n, err := migrate.Exec(db, dbDriverName, keystoreMigrations, migrate.Up) | ||
if err != nil { | ||
fmt.Fprintln(os.Stderr, "error applying up migrations", err) |
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.
Nit: the error string could be more readable. Currently, for an error err = "test error"
, "error applying up migrations test error" would be printed, which is slightly confusing. (https://play.golang.org/p/zH4020sdWOr for reference.) Consider: "error applying up migrations: %v", or something of the sort.
This applies to a variety of other places below - they can quickly be found by searching for os.Stderr
, and checking if the encapsulating string looks like fmt.Fprintln(os.Stderr, ..., err)
.
fmt.Fprintln(os.Stdout, id) | ||
} | ||
} else { | ||
fmt.Fprintln(os.Stdout, "All migrations have been unapplied!") |
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.
Should this text be "All migrations have been applied!"?
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.
LGTM!
This PR introduces
keystored migrate up/down
command to handlemigrations. It also defines an initial migration for the
encrypted_keys
table.