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

sqlite db migration fails on table already existing #224

Closed
rgarcia opened this issue Mar 26, 2014 · 2 comments · Fixed by #226
Closed

sqlite db migration fails on table already existing #224

rgarcia opened this issue Mar 26, 2014 · 2 comments · Fixed by #226

Comments

@rgarcia
Copy link

rgarcia commented Mar 26, 2014

Trying to upgrade to the latest version of drone and seeing this error on startup:

$ ./bin/droned
2014/03/26 13:32:28 Failed to upgrade to Revision Number 1
2014/03/26 13:32:28 table users already exists
2014/03/26 13:32:28 starting drone version 9f43d5c on port :8080

I changed

func (s *sqliteDriver) CreateTable(tableName string, args []string) (sql.Result, error)

to use CREATE TABLE IF NOT EXISTS, which seems more accurate. I'll submit a PR shortly.

@bradrydzewski
Copy link

@rgarcia thanks! @fudanchii will merge on your approval

@fudanchii
Copy link

Yeah, we should do this.
At first I was thinking not to suppress create table error, but I think it's a moot now since the error certainly hinder full-backward compatibility with tables which exist pre-migration. Also because Migration.up and Migration.down suppressed any error at migration step and replace it with Tx.Rollback, as mentioned by @rgarcia at #226. I think I missed this one since I ever experiment to bail out at migration step error:

if err := rev.Up(mg); err != nil {
                log.Printf("Failed to upgrade to Revision Number %v\n", current)
                log.Println(err)
                // suppress any possible error from tx.Rollback
                tx.Rollback()
                return err
}

But then I revert this and going with the old behavior, since this is will useful only if we extract the migration system from the main app (standalone application to do migration solely).

This could be merged in, I will fix mysql to adapt this. Thanks.

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 a pull request may close this issue.

3 participants