Migrate database up before serving using opm#100
Conversation
openshift-ci-robot
added
the
size/M
kevinrizza
left a comment
kevinrizza
left a comment
There was a problem hiding this comment.
Just a few comments, otherwise this looks good to me.
| return nil | ||
| } | ||
|
|
||
| func migrateToLatest(db *sql.DB) error { |
There was a problem hiding this comment.
I don't think this is the right place for this function. If we want to reuse it in other places (like when we add other commands to opm registry, maybe we should move it into the sqlite package? Otherwise we should just leave it in line.
There was a problem hiding this comment.
@kevinrizza this is a stop gap measure till we have the migrate command available with the opm cli. Once we have that this function can be removed and the corresponding function from the registry package should be used here
| // looks at the currently active migration version and will | ||
| // migrate all the way up (applying all up migrations). | ||
| func (m *SQLMigrator) MigrateUp() error { | ||
| instance, err := sqlite3.WithInstance(m.db, &sqlite3.Config{}) |
There was a problem hiding this comment.
This will make the migrator sqlite-only.
If we change
type SQLMigrator struct {
db *sql.DB
migrationsPath string
generated bool
}to
type SQLMigrator struct {
db database.Driver
migrationsPath string
generated bool
}then we can use the migrator for any db?
There was a problem hiding this comment.
We can make this change, but the golang-migrate needs that instance object to understand which database and explicitly support it. IMO we should probably revisit this later if we want to abstract the migrator to understand multiple database types.
There was a problem hiding this comment.
golang-migrate needs that instance object to understand which database and explicitly support it.
I don't think this is the case, but we can revisit it in the future.
I do think we should be careful to pass in a DatabaseName to sqlite3.Config, right?
There was a problem hiding this comment.
I do think we should be careful to pass in a DatabaseName to sqlite3.Config, right?
@ecordell I've updated the PR to pass in a DatabaseName. Why do you say careful though? I didn't seem to find any "best practice guide" around passing the database name in the docs so was curious.
- Add wrapper around Up migration - Call wrapper before serving database using opm cli
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anik120 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
|
Closing in favor or #101 |
4 participants
Description of the change:
Merge before #99
Reviewer Checklist
/docs