From ccf380d824948d69dc9230a259c37e8b164e8e9b Mon Sep 17 00:00:00 2001 From: Lorris Saint-Genez Date: Thu, 11 Jun 2026 14:44:54 -0700 Subject: [PATCH 1/2] feat(config): trigger the config.toml migration at startup ShouldMigrate fires when config.toml exists and state.toml doesn't: state.toml is only written on success, so its absence doubles as the "not migrated yet" marker and an aborted run retries on the next one. Migrate is a deliberate no-op until the migration body lands. Co-Authored-By: Claude Fable 5 --- pkg/cmd/root/root.go | 10 ++++++ pkg/config/migrate.go | 28 ++++++++++++++++ pkg/config/migrate_test.go | 68 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+) create mode 100644 pkg/config/migrate.go create mode 100644 pkg/config/migrate_test.go diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 069e7f28..fe70090b 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -136,6 +136,16 @@ func Execute() exitCode { cmdFactory := factory.New(version.Version, &cfg) stderr := cmdFactory.IOStreams.ErrOut + // One-time config.toml → state.toml + keychain migration (GROUT-363). + // Must run before the command executes: credential resolution reads + // state.toml once per invocation and caches it. A failed migration never + // blocks the command — state.toml stays absent, so it retries next run. + if cfg.ShouldMigrate() { + if err := cfg.Migrate(); err != nil && hasDebug { + fmt.Fprintf(stderr, "config migration failed (will retry on next run): %s\n", err) + } + } + // Set up the update notifier. updateMessageChan := make(chan *update.ReleaseInfo) go func() { diff --git a/pkg/config/migrate.go b/pkg/config/migrate.go new file mode 100644 index 00000000..c3e15631 --- /dev/null +++ b/pkg/config/migrate.go @@ -0,0 +1,28 @@ +package config + +import "os" + +// ShouldMigrate reports whether the one-time config.toml → state.toml + +// keychain migration still has to run: a legacy config.toml exists and +// state.toml does not. state.toml is only written when the migration (or any +// new-model write) succeeds, so its absence doubles as the "not migrated yet" +// marker — an aborted migration naturally retries on the next run. +func (c *Config) ShouldMigrate() bool { + if c.File == "" { + return false + } + if _, err := os.Stat(c.File); err != nil { + return false + } + return !c.StateFileExists() +} + +// 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. +func (c *Config) Migrate() error { + return nil +} diff --git a/pkg/config/migrate_test.go b/pkg/config/migrate_test.go new file mode 100644 index 00000000..6aa7115a --- /dev/null +++ b/pkg/config/migrate_test.go @@ -0,0 +1,68 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConfig_ShouldMigrate(t *testing.T) { + tests := []struct { + name string + configFile bool + stateFile bool + want bool + }{ + { + name: "config.toml only: migration pending", + configFile: true, + stateFile: false, + want: true, + }, + { + name: "both files: already migrated (or new model in use)", + configFile: true, + stateFile: true, + want: false, + }, + { + name: "state.toml only: nothing to migrate", + configFile: false, + stateFile: true, + want: false, + }, + { + name: "no files: fresh install", + configFile: false, + stateFile: false, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + cfg := &Config{ + File: filepath.Join(dir, "config.toml"), + StateFile: filepath.Join(dir, "state.toml"), + } + if tt.configFile { + require.NoError(t, os.WriteFile(cfg.File, []byte(""), 0o600)) + } + if tt.stateFile { + require.NoError(t, os.WriteFile(cfg.StateFile, []byte(""), 0o600)) + } + + assert.Equal(t, tt.want, cfg.ShouldMigrate()) + }) + } +} + +func TestConfig_ShouldMigrate_unresolvedPaths(t *testing.T) { + // InitConfig never ran: paths are empty, the trigger must stay off. + cfg := &Config{} + assert.False(t, cfg.ShouldMigrate()) +} From d73969b7f2bcd2436f4e891a861e36772451fb01 Mon Sep 17 00:00:00 2001 From: Lorris Saint-Genez Date: Fri, 12 Jun 2026 14:08:47 -0700 Subject: [PATCH 2/2] feat(config): migrate config.toml profiles to the keychain and state.toml (#246) Co-authored-by: Claude Fable 5 --- pkg/config/migrate.go | 96 ++++++++++++++++- pkg/config/migrate_test.go | 211 +++++++++++++++++++++++++++++++++++++ 2 files changed, 302 insertions(+), 5 deletions(-) diff --git a/pkg/config/migrate.go b/pkg/config/migrate.go index c3e15631..54949f25 100644 --- a/pkg/config/migrate.go +++ b/pkg/config/migrate.go @@ -1,6 +1,14 @@ package config -import "os" +import ( + "os" + "sort" + + log "github.com/sirupsen/logrus" + "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 +28,87 @@ 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.migratableProfiles() { + 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) +} + +// 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 6aa7115a..1786dc19 100644 --- a/pkg/config/migrate_test.go +++ b/pkg/config/migrate_test.go @@ -5,8 +5,13 @@ 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" + "github.com/zalando/go-keyring" + + "github.com/algolia/cli/pkg/keychain" ) func TestConfig_ShouldMigrate(t *testing.T) { @@ -66,3 +71,209 @@ 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()) +} + +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) +}