From 9cce91d93d550f418341bd3c35eac5d490010dcc Mon Sep 17 00:00:00 2001 From: Lorris Saint-Genez Date: Fri, 12 Jun 2026 13:42:05 -0700 Subject: [PATCH 1/2] feat(config): migrate config.toml profiles to the keychain and state.toml Per profile: api_key (+ crawler_api_key if set) goes to the OS keychain under the profile's application ID, state.toml gets an entry with alias = profile name, and current_application_id is taken from the default = true profile. api_key_uuid stays empty for legacy keys. Keychain first, state.toml last (atomic temp + rename): a keychain failure leaves state.toml absent so the migration retries on the next run. An empty config.toml still writes an empty state.toml so the trigger turns off. Co-Authored-By: Claude Fable 5 --- pkg/config/migrate.go | 38 +++++++++++-- pkg/config/migrate_test.go | 112 +++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 5 deletions(-) diff --git a/pkg/config/migrate.go b/pkg/config/migrate.go index c3e15631..cfee2cc3 100644 --- a/pkg/config/migrate.go +++ b/pkg/config/migrate.go @@ -1,6 +1,12 @@ package config -import "os" +import ( + "os" + + "github.com/spf13/viper" + + "github.com/algolia/cli/pkg/keychain" +) // ShouldMigrate reports whether the one-time config.toml → state.toml + // keychain migration still has to run: a legacy config.toml exists and @@ -20,9 +26,31 @@ func (c *Config) ShouldMigrate() bool { // Migrate moves the legacy config.toml profiles into the new model (state.toml // + OS keychain). config.toml itself is never modified. // -// The migration body lands in follow-up PRs; until then this is a no-op that -// deliberately does NOT write state.toml, so ShouldMigrate keeps returning -// true and the real migration will run once shipped. +// Secrets go to the keychain first; state.toml is only written — atomically, +// via State.Save's temp + rename — once every profile's keys are stored. A +// keychain failure mid-run therefore leaves state.toml absent and the whole +// migration retries on the next command; entries already written are simply +// rewritten then. With nothing to migrate an empty state.toml still gets +// written, so ShouldMigrate stops firing on every command. func (c *Config) Migrate() error { - return nil + state := &State{Applications: map[string]ApplicationState{}} + + for _, profile := range c.ConfiguredProfiles() { + secrets := keychain.AppSecrets{ + APIKey: profile.APIKey, + CrawlerAPIKey: viper.GetString(profile.GetFieldName("crawler_api_key")), + } + if err := keychain.SaveAppSecrets(profile.ApplicationID, secrets); err != nil { + return err + } + + // api_key_uuid is unknown for legacy keys: left empty until an API + // lookup can backfill it. + state.UpsertApplication(profile.ApplicationID, ApplicationState{Alias: profile.Name}) + if profile.Default { + state.SetCurrentApplication(profile.ApplicationID) + } + } + + return state.Save(c.StateFile) } diff --git a/pkg/config/migrate_test.go b/pkg/config/migrate_test.go index 6aa7115a..bcb08c5c 100644 --- a/pkg/config/migrate_test.go +++ b/pkg/config/migrate_test.go @@ -5,8 +5,12 @@ import ( "path/filepath" "testing" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/zalando/go-keyring" + + "github.com/algolia/cli/pkg/keychain" ) func TestConfig_ShouldMigrate(t *testing.T) { @@ -66,3 +70,111 @@ func TestConfig_ShouldMigrate_unresolvedPaths(t *testing.T) { cfg := &Config{} assert.False(t, cfg.ShouldMigrate()) } + +// migrationConfig writes a config.toml with the given content, points the +// global viper at it (ConfiguredProfiles reads through viper) and returns a +// Config ready to migrate. +func migrationConfig(t *testing.T, content string) *Config { + t.Helper() + + dir := t.TempDir() + configFile := filepath.Join(dir, "config.toml") + require.NoError(t, os.WriteFile(configFile, []byte(content), 0o600)) + + viper.Reset() + viper.SetConfigType("toml") + viper.SetConfigFile(configFile) + require.NoError(t, viper.ReadInConfig()) + t.Cleanup(viper.Reset) + + return &Config{ + File: configFile, + StateFile: filepath.Join(dir, "state.toml"), + } +} + +func TestConfig_Migrate(t *testing.T) { + keyring.MockInit() + cfg := migrationConfig(t, `[prod] +application_id = "APP1" +api_key = "key-1" +crawler_api_key = "crawler-1" +default = true + +[dev] +application_id = "APP2" +api_key = "key-2" +`) + + require.NoError(t, cfg.Migrate()) + + // Secrets land in the keychain, crawler key included when set. + prod, err := keychain.LoadAppSecrets("APP1") + require.NoError(t, err) + require.NotNil(t, prod) + assert.Equal(t, "key-1", prod.APIKey) + assert.Equal(t, "crawler-1", prod.CrawlerAPIKey) + + dev, err := keychain.LoadAppSecrets("APP2") + require.NoError(t, err) + require.NotNil(t, dev) + assert.Equal(t, "key-2", dev.APIKey) + assert.Empty(t, dev.CrawlerAPIKey) + + // state.toml: one entry per application, alias = profile name, current + // application = the default profile's one. + st, err := LoadState(cfg.StateFile) + require.NoError(t, err) + assert.Equal(t, "APP1", st.CurrentApplicationID) + assert.Equal(t, "prod", st.Applications["APP1"].Alias) + assert.Equal(t, "dev", st.Applications["APP2"].Alias) + assert.Empty(t, st.Applications["APP1"].APIKeyUUID) // unknown for legacy keys + + // state.toml now exists: the trigger turns off. + assert.False(t, cfg.ShouldMigrate()) +} + +func TestConfig_Migrate_NoDefaultProfileLeavesCurrentEmpty(t *testing.T) { + keyring.MockInit() + cfg := migrationConfig(t, `[dev] +application_id = "APP2" +api_key = "key-2" +`) + + require.NoError(t, cfg.Migrate()) + + st, err := LoadState(cfg.StateFile) + require.NoError(t, err) + assert.Empty(t, st.CurrentApplicationID) + assert.Equal(t, "dev", st.Applications["APP2"].Alias) +} + +func TestConfig_Migrate_EmptyConfigStillWritesState(t *testing.T) { + keyring.MockInit() + cfg := migrationConfig(t, "") + + require.NoError(t, cfg.Migrate()) + + // An empty state.toml must exist, otherwise the migration would re-run + // (and re-log) on every command. + st, err := LoadState(cfg.StateFile) + require.NoError(t, err) + assert.Empty(t, st.CurrentApplicationID) + assert.Empty(t, st.Applications) + assert.False(t, cfg.ShouldMigrate()) +} + +func TestConfig_Migrate_KeychainFailureLeavesStateAbsent(t *testing.T) { + keyring.MockInitWithError(keyring.ErrUnsupportedPlatform) + cfg := migrationConfig(t, `[prod] +application_id = "APP1" +api_key = "key-1" +`) + + require.Error(t, cfg.Migrate()) + + // state.toml untouched: ShouldMigrate keeps firing so the migration + // retries on the next run. + assert.NoFileExists(t, cfg.StateFile) + assert.True(t, cfg.ShouldMigrate()) +} From f252ba49a69e63d27a769bb895993ce83af0215c Mon Sep 17 00:00:00 2001 From: Lorris Saint-Genez Date: Fri, 12 Jun 2026 14:08:32 -0700 Subject: [PATCH 2/2] feat(config): apply skip rules during the config.toml migration (#247) Co-authored-by: Claude Fable 5 --- pkg/config/migrate.go | 60 ++++++++++++++++++++++- pkg/config/migrate_test.go | 99 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 1 deletion(-) diff --git a/pkg/config/migrate.go b/pkg/config/migrate.go index cfee2cc3..54949f25 100644 --- a/pkg/config/migrate.go +++ b/pkg/config/migrate.go @@ -2,7 +2,9 @@ package config import ( "os" + "sort" + log "github.com/sirupsen/logrus" "github.com/spf13/viper" "github.com/algolia/cli/pkg/keychain" @@ -35,7 +37,7 @@ func (c *Config) ShouldMigrate() bool { func (c *Config) Migrate() error { state := &State{Applications: map[string]ApplicationState{}} - for _, profile := range c.ConfiguredProfiles() { + for _, profile := range c.migratableProfiles() { secrets := keychain.AppSecrets{ APIKey: profile.APIKey, CrawlerAPIKey: viper.GetString(profile.GetFieldName("crawler_api_key")), @@ -54,3 +56,59 @@ func (c *Config) Migrate() error { return state.Save(c.StateFile) } + +// migratableProfiles applies the migration skip rules to the config.toml +// profiles before any keychain write happens: +// +// - admin_api_key never moves to the new model: one log line points to its +// replacements, whether the profile migrates or not. +// - A profile without application_id or with an empty api_key has nothing +// usable to migrate: skipped with a log line. +// - Profiles sharing the same application_id would overwrite each other's +// keychain entry: the default = true profile wins, the others are logged +// as conflicts and skipped. +// +// Profiles are processed in name order so conflict resolution and logs stay +// deterministic (ConfiguredProfiles iterates a map). +func (c *Config) migratableProfiles() []*Profile { + profiles := c.ConfiguredProfiles() + sort.Slice(profiles, func(i, j int) bool { return profiles[i].Name < profiles[j].Name }) + + selected := make([]*Profile, 0, len(profiles)) + owner := map[string]int{} // application ID → index in selected + + for _, profile := range profiles { + if profile.AdminAPIKey != "" { + log.Warnf( + "config migration: profile %q: admin_api_key is not migrated, use ALGOLIA_ADMIN_API_KEY or --api-key instead", + profile.Name, + ) + } + if profile.ApplicationID == "" { + log.Warnf("config migration: skipping profile %q: no application_id", profile.Name) + continue + } + if profile.APIKey == "" { + log.Warnf("config migration: skipping profile %q: empty api_key", profile.Name) + continue + } + if i, ok := owner[profile.ApplicationID]; ok { + kept, dropped := selected[i], profile + if profile.Default && !kept.Default { + selected[i] = profile + kept, dropped = profile, kept + } + log.Warnf( + "config migration: skipping profile %q: application %q already migrated from profile %q", + dropped.Name, + dropped.ApplicationID, + kept.Name, + ) + continue + } + owner[profile.ApplicationID] = len(selected) + selected = append(selected, profile) + } + + return selected +} diff --git a/pkg/config/migrate_test.go b/pkg/config/migrate_test.go index bcb08c5c..1786dc19 100644 --- a/pkg/config/migrate_test.go +++ b/pkg/config/migrate_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + logtest "github.com/sirupsen/logrus/hooks/test" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -178,3 +179,101 @@ api_key = "key-1" assert.NoFileExists(t, cfg.StateFile) assert.True(t, cfg.ShouldMigrate()) } + +func TestConfig_Migrate_SkipRules(t *testing.T) { + keyring.MockInit() + hook := logtest.NewGlobal() + t.Cleanup(hook.Reset) + + cfg := migrationConfig(t, `[nokey] +application_id = "APP3" +api_key = "" + +[noapp] +api_key = "key-x" + +[adminonly] +application_id = "APP4" +admin_api_key = "admin-key" +`) + + require.NoError(t, cfg.Migrate()) + + // Nothing migrated: no keychain entries, an empty state.toml that still + // turns the trigger off. + for _, appID := range []string{"APP3", "APP4"} { + secrets, err := keychain.LoadAppSecrets(appID) + require.NoError(t, err) + assert.Nil(t, secrets) + } + st, err := LoadState(cfg.StateFile) + require.NoError(t, err) + assert.Empty(t, st.Applications) + assert.False(t, cfg.ShouldMigrate()) + + // Each skip got its log line, plus the admin_api_key notice. + logs := make([]string, 0, len(hook.AllEntries())) + for _, entry := range hook.AllEntries() { + logs = append(logs, entry.Message) + } + assert.Contains(t, logs, + `config migration: skipping profile "nokey": empty api_key`) + assert.Contains(t, logs, + `config migration: skipping profile "noapp": no application_id`) + assert.Contains(t, logs, + `config migration: skipping profile "adminonly": empty api_key`) + assert.Contains( + t, + logs, + `config migration: profile "adminonly": admin_api_key is not migrated, use ALGOLIA_ADMIN_API_KEY or --api-key instead`, + ) +} + +func TestConfig_Migrate_DuplicateApplicationKeepsDefault(t *testing.T) { + keyring.MockInit() + cfg := migrationConfig(t, `[backup] +application_id = "APP1" +api_key = "backup-key" + +[prod] +application_id = "APP1" +api_key = "prod-key" +default = true +`) + + require.NoError(t, cfg.Migrate()) + + // One single entry for APP1: the default profile's alias and key. + st, err := LoadState(cfg.StateFile) + require.NoError(t, err) + require.Len(t, st.Applications, 1) + assert.Equal(t, "prod", st.Applications["APP1"].Alias) + assert.Equal(t, "APP1", st.CurrentApplicationID) + + secrets, err := keychain.LoadAppSecrets("APP1") + require.NoError(t, err) + require.NotNil(t, secrets) + assert.Equal(t, "prod-key", secrets.APIKey) +} + +func TestConfig_Migrate_AdminKeyAlongsideAPIKeyStillMigrates(t *testing.T) { + keyring.MockInit() + cfg := migrationConfig(t, `[prod] +application_id = "APP1" +api_key = "key-1" +admin_api_key = "admin-1" +default = true +`) + + require.NoError(t, cfg.Migrate()) + + // The search key migrates; the admin key has no slot in the new model. + secrets, err := keychain.LoadAppSecrets("APP1") + require.NoError(t, err) + require.NotNil(t, secrets) + assert.Equal(t, "key-1", secrets.APIKey) + + st, err := LoadState(cfg.StateFile) + require.NoError(t, err) + assert.Equal(t, "prod", st.Applications["APP1"].Alias) +}