Retire multi-account migration - #14430
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Removing the fallback migration can leave legacy configurations inaccessible or cause existing credentials to be lost.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
internal/config/migration/multi_account.go — Retaining legacy configs can lose the existing account token |
|
internal/ghcmd/cmd.go — Versionless legacy configs are no longer migrated |
|
internal/config/migration/multi_account.go — Update documentation for the retired migration |
What changed in this PR
Retires automatic execution of the legacy multi-account migration while retaining its implementation.
Changes:
- Removes startup migration invocation.
- Renames the deprecated migration type.
- Updates migration and authentication tests.
| File | Summary |
|---|---|
internal/ghcmd/cmd.go |
Removes automatic migration execution. |
internal/config/migration/multi_account.go |
Renames and retains the deprecated migration. |
internal/config/migration/multi_account_test.go |
Updates migration tests. |
internal/config/auth_config_test.go |
Updates authentication tests. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // This migration no longer runs automatically. Its compatibility window | ||
| // covered upgrades from versions that predated the multi-account schema. | ||
| // Supported configurations now already use that schema, so running it before | ||
| // every command no longer provides value. The implementation remains as | ||
| // executable documentation of the original migration and its compatibility |
There was a problem hiding this comment.
Yes I don't think we care about users that are on a pre v2.40 release, it's 3 years old, broken in many ways.
| @@ -133,14 +132,6 @@ func Main() exitCode { | |||
|
|
|||
| cmdFactory := factory.New(buildVersion, string(invokingAgent), cfgFunc, ioStreams, ghExecutablePath, telemetryService) | |||
|
|
|||
There was a problem hiding this comment.
Yes I don't think we care about users that are on a pre v2.40 release, it's 3 years old, broken in many ways.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>


Description
When initial multi account support was added to
ghinv2.40.0(December 2023, so almost 3 years ago at this point), we had to perform some data migrations. The code for this, with lots of nice explanations can be found in multi-account.go, and it has been running on every invocation ofghfor 3 years:cli/internal/ghcmd/cmd.go
Lines 136 to 142 in 7f0590c
The TL;DR was that we needed to take host level user/token data and namespace it by username. For example, a hosts entry of:
would become:
A similar operation would occur for the keyring with a key of:
being copied to:
"gh:" + hostname + ":" + username
This set the stage for
gh auth switchto pick the user and token from a list and "activate" them in the config/keyring entry. The data format was both backwards and forwards compatible, to allow users to move back and forward between versions without breaking anything. However, this global keyring entry has been the source of a number of bugs such as:gh auth token --secure-storageignores the config's active account and returns the globally-active keyring token #14370These bugs occur because people use
GH_CONFIG_DIRto swap config files (and therefore active users). This can cause a split brain in some cases where the active user in the config, and the user owning the token in the keyring aren't the same. We can fix the code paths, but we're now 3 years on and I don't think we should give much concern to compatability prev2.40.0, so I want to remove that keyring entry altogether.This PR doesn't do that work, but it does remove the migration that circles the area and just sits there as debt.
How did you test this change?
Authorship and follow-up
Who wrote this:
Who answers review comments: