Improve error message in case no migration is found #158
Conversation
Pull Request Test Coverage Report for Build 287
💛 - Coveralls |
Pull Request Test Coverage Report for Build 532
💛 - Coveralls |
| @@ -753,7 +753,7 @@ func (m *Migrate) versionExists(version uint) error { | |||
| return err | |||
| } | |||
|
|
|||
| return os.ErrNotExist | |||
There was a problem hiding this comment.
I love the idea of showing which migration failed but this is a backwards incompatible change. e.g. there could be a bunch of code that relies on it as shown by all of your test changes below
A cleaner solution may be to add a logging statement here.
There was a problem hiding this comment.
Ok, now it returns a similar error so the tests do not change
There was a problem hiding this comment.
Using os.PathError is a much better approach!
However, this is still not a backwards compatible change since existing checks against os.ErrNotExist will fail in this case. e.g. existing checks will need to use os.IsNotExists() instead. The unit tests happen to already be using os.IsNotExists().
Do you have a test case or scenario (not in the unittests where the sources are stubbed) in which a os.PathError will be returned?
migrate.go
Outdated
| @@ -753,6 +753,7 @@ func (m *Migrate) versionExists(version uint) error { | |||
| return err | |||
| } | |||
|
|
|||
| m.logPrintf("ERROR: No migration found for version %d", version) | |||
There was a problem hiding this comment.
The convention in this code base is to use error: instead of ERROR:
|
+1 for logging a proper message here, as |
|
@freeyoung Feel free to open a new PR that supersedes this one. |
github.com/golang-migrate/migrate is upgraded from v4.3.1 to v4.6.2, which provides the following features that are useful: * Running `migrate down` now prompts a confirmation, so that you can't accidentally drop the whole database. `migrate down -all` is available to replicate the old behavior * Improved error messages (golang-migrate/migrate#158) Details are at https://github.com/golang-migrate/migrate/releases. Change-Id: I9e942f0c164a0fd62ed2e74c949da0c10d1b8d97 Reviewed-on: https://team-review.git.corp.google.com/c/golang/discovery/+/549075 Reviewed-by: Robert Findley <rfindley@google.com>
In case no migration is found for the current version, we got this error
Now we get an error like this
Fixes #79