Update HasPending logic to account for out-of-order errors #762
Conversation
| check.Number(t, target, len(fsys)) | ||
| _, err = p.HasPending(ctx) | ||
| check.HasError(t, err) | ||
| check.Contains(t, err.Error(), "missing (out-of-order) migration") |
There was a problem hiding this comment.
The goal was to optimize the pending check by only checking the latest migration, but this is not correct since the default behavior SHOULD BE to fail if there are missing (out-of-order) migrations.
And we can only know if there are missing migrations by checking all of them. I wish there was a better way.
Maybe an "ignore missing" option for those who enforce behavior in CI pipelines?
| check.NoError(t, err) | ||
| // TODO(mf): revisit the pending check behavior in addition to the HasPending | ||
| // method. | ||
| current, target, err := p.CheckPending(ctx) |
There was a problem hiding this comment.
Given CheckPending only gets the current (db) and target (fsys) versions, I'm starting to reconsider the naming. I really don't want to give users the impression that it can be used for "pending" checks.
Some alternatives
- VersionCheck
PendingVersion- CompareVersion
- GetVersions (there is a
GetDBVersion) this might be a compliment - CurrentAndTarget
There was a problem hiding this comment.
Will tackle this in a separate PR, it's unreleased yet, so a breaking change here may be warranted. I think it'll just be a name change, leaning towards GetVersions() (current, target int64, err error).
EDIT: for brevity #764
Fix #761
This PR reverts the optimization in #758 and uses the same up logic to check whether all migrations have been applied. I.e., if allow missing (out-of-order) is disabled, which is the default, we report an error instead of false and
nilerror.This avoids silently ignoring missing migrations, which is typically true for timestamp-based migrations.
I do wish we could optimize this because, for those who convert timestamp -> sequential hybrid migrations and gate for the correct ordering in CI, it's not possible to get into this state. It'd be nice for those folks to benefit from more efficient checks along this code path.