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

@@ -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

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.

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()))

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
 	}

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
 }

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 {