From 135318f542d7cdc1362b7bab111add4a0fe614cb Mon Sep 17 00:00:00 2001 From: Neil O'Toole Date: Mon, 29 Jan 2024 22:03:03 -0700 Subject: [PATCH] sq fails on first config write (#387) * Fixed bug with config write on fresh install * Added test for config write on fresh install --- CHANGELOG.md | 13 +++++++++++++ cli/cli_test.go | 23 +++++++++++++++++++++++ cli/config/store.go | 8 ++++++++ cli/config/yamlstore/defaultload.go | 2 +- cli/config/yamlstore/yamlstore.go | 4 ++-- cli/run.go | 26 +++++++++++++++----------- 6 files changed, 62 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e8b8433..e806cf81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Breaking changes are annotated with ☢️, and alpha/beta features with 🐥. +## [v0.47.2] - 2024-01-29 + +Yet another morning-after-the-big-release issue, a nasty little one this time. +See the earlier [`v0.47.0`](https://github.com/neilotoole/sq/releases/tag/v0.47.0) release +for recent headline features. + +### Fixed + +- `sq` was failing to write config when there was no pre-existing config file. This was due to + a bug in the newly-introduced (as of `v0.47.0`) config locking mechanism. Fixed. + + ## [v0.47.1] - 2024-01-29 This is a tiny bugfix release for a runtime issue on some Linux distros. See @@ -1103,3 +1115,4 @@ make working with lots of sources much easier. [v0.46.1]: https://github.com/neilotoole/sq/compare/v0.46.0...v0.46.1 [v0.47.0]: https://github.com/neilotoole/sq/compare/v0.46.1...v0.47.0 [v0.47.1]: https://github.com/neilotoole/sq/compare/v0.47.0...v0.47.1 +[v0.47.2]: https://github.com/neilotoole/sq/compare/v0.47.1...v0.47.2 diff --git a/cli/cli_test.go b/cli/cli_test.go index 5e021897..7df8dd3c 100644 --- a/cli/cli_test.go +++ b/cli/cli_test.go @@ -10,6 +10,8 @@ import ( "strings" "testing" + "github.com/neilotoole/sq/cli/config" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -26,6 +28,27 @@ import ( "github.com/neilotoole/sq/testh/tu" ) +// TestConfigWriteOnFreshInstall verifies that sq can write +// to the config store when there is no existing config. +func TestConfigWriteOnFreshInstall(t *testing.T) { + cfgDir := tu.TempDir(t) + t.Setenv(config.EnvarConfigDir, cfgDir) + ctx := context.Background() + + // We want to write to the config, it doesn't matter what + // we write, just that we write something. + args := []string{"config", "set", cli.OptPingCmdTimeout.Key(), "3m33s"} + + err := cli.Execute(ctx, os.Stdin, os.Stdout, os.Stderr, args) + require.NoError(t, err) + + args = []string{"config", "get", cli.OptPingCmdTimeout.Key()} + buf := &bytes.Buffer{} + err = cli.Execute(ctx, os.Stdin, buf, os.Stderr, args) + require.NoError(t, err) + require.Equal(t, "3m33s\n", buf.String()) +} + func TestSmoke(t *testing.T) { t.Parallel() // Execute a bunch of smoke test cases. diff --git a/cli/config/store.go b/cli/config/store.go index 80443173..6e247dbc 100644 --- a/cli/config/store.go +++ b/cli/config/store.go @@ -13,6 +13,9 @@ import ( // Store saves and loads config. type Store interface { + // Exists returns true if the config exists in the store. + Exists() bool + // Save writes config to the store. Save(ctx context.Context, cfg *Config) error @@ -34,6 +37,11 @@ type Store interface { // and Load always returns a new empty Config. Useful for testing. type DiscardStore struct{} +// Exists implements Store.Exists. It returns true. +func (s DiscardStore) Exists() bool { + return true +} + // Lockfile implements Store.Lockfile. func (DiscardStore) Lockfile() (lockfile.Lockfile, error) { f, err := os.CreateTemp("", fmt.Sprintf("sq-%d.lock", os.Getpid())) diff --git a/cli/config/yamlstore/defaultload.go b/cli/config/yamlstore/defaultload.go index 66760796..0ae128cd 100644 --- a/cli/config/yamlstore/defaultload.go +++ b/cli/config/yamlstore/defaultload.go @@ -48,7 +48,7 @@ func Load(ctx context.Context, osArgs []string, optsReg *options.Registry, OptionsRegistry: optsReg, } - if !cfgStore.fileExists() { + if !cfgStore.Exists() { cfg := config.New() return cfg, cfgStore, nil } diff --git a/cli/config/yamlstore/yamlstore.go b/cli/config/yamlstore/yamlstore.go index 0eee1f52..6d6c738a 100644 --- a/cli/config/yamlstore/yamlstore.go +++ b/cli/config/yamlstore/yamlstore.go @@ -209,9 +209,9 @@ func (fs *Store) write(ctx context.Context, data []byte) error { return nil } -// fileExists returns true if the backing file can be accessed, false if it doesn't +// Exists returns true if the backing file can be accessed, false if it doesn't // exist or on any error. -func (fs *Store) fileExists() bool { +func (fs *Store) Exists() bool { _, err := os.Stat(fs.Path) return err == nil } diff --git a/cli/run.go b/cli/run.go index 4f863c17..23498e98 100644 --- a/cli/run.go +++ b/cli/run.go @@ -348,7 +348,9 @@ func cmdRequiresConfigLock(cmd *cobra.Command) bool { // lockReloadConfig acquires the lock for the config store, and updates the // run (as found on cmd's context) with a fresh copy of the config, loaded -// after lock acquisition. +// after lock acquisition. If there's no config persisted in the store, +// the run's config is left untouched. (That run config will later be +// saved to the store, if appropriate.) // // The config lock should be acquired before making any changes to config. // Timeout and progress options from ctx are honored. @@ -387,18 +389,20 @@ func lockReloadConfig(cmd *cobra.Command) (unlock func(), err error) { return nil, errz.Wrap(err, "acquire config lock") } - var cfg *config.Config - if cfg, err = ru.ConfigStore.Load(ctx); err != nil { - // An error occurred reloading config; release the lock before returning. - if unlockErr := lock.Unlock(); unlockErr != nil { - lg.FromContext(ctx).Warn("Failed to release config lock", - lga.Lock, lock, lga.Err, unlockErr) + if ru.ConfigStore.Exists() { + var cfg *config.Config + if cfg, err = ru.ConfigStore.Load(ctx); err != nil { + // An error occurred reloading config; release the lock before returning. + if unlockErr := lock.Unlock(); unlockErr != nil { + lg.FromContext(ctx).Warn("Failed to release config lock", + lga.Lock, lock, lga.Err, unlockErr) + } + return nil, err } - return nil, err - } - // Update the run with the fresh config. - ru.Config = cfg + // Assign the newly-reloaded config to the run. + ru.Config = cfg + } // Else, the config doesn't currently exist on disk; no reload required. return func() { if unlockErr := lock.Unlock(); unlockErr != nil {