graphqlbackend: check requireed OOB migrations against latest version#48053
Conversation
| if updateStatus == nil || !updateStatus.HasUpdate() { | ||
| return nil, errors.New("no latest update version available (reload in a few seconds)") | ||
| } | ||
| version, _, ok := oobmigration.NewVersionAndPatchFromString(updateStatus.UpdateVersion) |
There was a problem hiding this comment.
We want to alert if an unfinished migration is deprecated at or above this value, right? In that case we don't want to use either the current version (previous) or the update version (which is the latest release, if I'm reading correctly), but we want the next version to see if any update will require additional migrations.
I think what we want here is just version.Next() (assuming version is an oobmigration.Version, then we handle the major version transition already).
Thoughts?
There was a problem hiding this comment.
We want to alert if an unfinished migration is deprecated at or above this value, right?
Yes, but without asking the user (which we can), how could we know what version the user intends to upgrade to?
If we only check for the "next" version, how would we solve the following scenario:
- The current version is N, and the next version is N+1.
- An OOBM is added in N but deprecated at N+2.
- N+2 is the latest version that the user intends to upgrade to.
Only checking against N+1 seems would give the wrong conclusion for readiness? 🤔
There was a problem hiding this comment.
Good point. Are we only considering migrations which were introduced before the current migration? If not then we might say "this migration that doesn't exist at this release is at 0%".
There was a problem hiding this comment.
Are we only considering migrations which were introduced before the current migration?
I feel this is the best we can do, when there are OOBMs that are:
- started
- not yet done
- known to be deprecated using the most conservative algorithm (i.e. checking against the latest version)
indicate definitely not ready, this way we can at least be able to inform and eliminate obvious problems.
WDYT?
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 63f7afb...d27e40a.
|
Previously, we were checking agsinst the current running version for required OOB migrations, which is incorrect as pointed out in https://github.com/sourcegraph/sourcegraph/pull/48046#discussion_r1114467855 because if an unfinished OOBM is already deprecated at the current running version the instance won't even start.
Instead, we should be checking against the latest version, (reasonably) taking the assumption that the site admin always wants to upgrade from the current to the latest version (i.e. what's multi-version upgrade is for).
Test plan
Randomly setting the progress some deprecated OOBM to 0.