Skip to content
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

Merged
merged 6 commits into from
May 8, 2019

Conversation

howardtw
Copy link
Contributor

@howardtw howardtw commented May 7, 2019

This PR introduces keystored migrate up/down command to handle
migrations. It also defines an initial migration for the encrypted_keys
table.

This PR introduces `keystored migrate up/down` command to handle
migrations. It also defines an initial migration for the encrypted_keys
table.
@howardtw howardtw requested review from debnil and tomquisel May 7, 2019 23:06
Copy link
Contributor

@debnil debnil left a 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:
Copy link
Contributor

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)
Copy link
Contributor

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!")
Copy link
Contributor

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!"?

Copy link
Contributor

@debnil debnil left a comment

Choose a reason for hiding this comment

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

LGTM!

@howardtw howardtw merged commit 5a3253c into stellar:master May 8, 2019
@howardtw howardtw deleted the keystore-migration branch May 8, 2019 19:58
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.

2 participants