Skip to content

Commit 8f193d9

Browse files
murrayjumurrayju
authored
AGE-182: only write explicit changes to config file (#36)
The config system had a problem where the _effective_ config (after resolving CLI args, env vars, and defaults) would be written to the config file when _any_ config was intended to be saved. This caused several problems, most annoying of which being that temporarily passed CLI flags would get persisted (like `-o json`). The solution is to never use the global `viper` instance to write the config file. Instead, a new instance is created, and only populated with the *file* content before being updated as needed. Then this can be written without carrying in unwanted values.
1 parent ee2c06d commit 8f193d9

File tree

10 files changed

+617
-375
lines changed

10 files changed

+617
-375
lines changed

‎.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,6 @@ config.yaml
4949

5050
# Build artifacts
5151
latest.txt
52+
53+
# Claude
54+
/.claude/*.local.*

‎internal/tiger/cmd/auth_test.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import (
1616
"testing"
1717
"time"
1818

19-
"github.com/spf13/viper"
2019
"github.com/timescale/tiger-cli/internal/tiger/config"
2120
)
2221

@@ -44,8 +43,9 @@ func setupAuthTest(t *testing.T) string {
4443
os.Setenv("TIGER_CONFIG_DIR", tmpDir)
4544

4645
// Reset global config and viper to ensure test isolation
47-
config.ResetGlobalConfig()
48-
viper.Reset()
46+
if _, err := config.UseTestConfig(tmpDir, map[string]any{}); err != nil {
47+
t.Fatalf("Failed to use test config: %v", err)
48+
}
4949

5050
// Also ensure config file doesn't exist
5151
configFile := config.GetConfigFile(tmpDir)
@@ -56,7 +56,6 @@ func setupAuthTest(t *testing.T) string {
5656
config.RemoveAPIKeyFromKeyring()
5757
// Reset global config and viper first
5858
config.ResetGlobalConfig()
59-
viper.Reset()
6059
validateAPIKeyForLogin = originalValidator // Restore original validator
6160
// Remove config file explicitly
6261
configFile := config.GetConfigFile(tmpDir)

‎internal/tiger/cmd/config_test.go‎

Lines changed: 71 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"strings"
99
"testing"
1010

11-
"github.com/spf13/viper"
1211
"gopkg.in/yaml.v3"
1312

1413
"github.com/timescale/tiger-cli/internal/tiger/config"
@@ -17,12 +16,6 @@ import (
1716
func setupConfigTest(t *testing.T) (string, func()) {
1817
t.Helper()
1918

20-
// Reset viper state to ensure clean test environment
21-
viper.Reset()
22-
23-
// Reset global config in the config package
24-
config.ResetGlobalConfig()
25-
2619
// Create temporary directory for test config
2720
tmpDir, err := os.MkdirTemp("", "tiger-config-test-*")
2821
if err != nil {
@@ -32,11 +25,12 @@ func setupConfigTest(t *testing.T) (string, func()) {
3225
// Set environment variable to use test directory
3326
os.Setenv("TIGER_CONFIG_DIR", tmpDir)
3427

28+
config.UseTestConfig(tmpDir, map[string]any{})
29+
3530
// Clean up function
3631
cleanup := func() {
3732
os.RemoveAll(tmpDir)
3833
os.Unsetenv("TIGER_CONFIG_DIR")
39-
viper.Reset()
4034

4135
// Reset global config in the config package
4236
// This is important for test isolation
@@ -563,18 +557,20 @@ func TestConfigReset(t *testing.T) {
563557
t.Errorf("Expected output to contain reset message, got '%s'", strings.TrimSpace(output))
564558
}
565559

566-
// Verify all values were reset to defaults
567560
cfg, err = config.Load()
568561
if err != nil {
569562
t.Fatalf("Failed to load config: %v", err)
570563
}
571564

565+
// ProjectID should be preserved
566+
if cfg.ProjectID != "custom-project" {
567+
t.Errorf("Expected ProjectID %s, got %s", "custom-project", cfg.ProjectID)
568+
}
569+
570+
// Verify all other values were reset to defaults
572571
if cfg.APIURL != config.DefaultAPIURL {
573572
t.Errorf("Expected default APIURL %s, got %s", config.DefaultAPIURL, cfg.APIURL)
574573
}
575-
if cfg.ProjectID != "" {
576-
t.Errorf("Expected empty ProjectID, got %s", cfg.ProjectID)
577-
}
578574
if cfg.ServiceID != "" {
579575
t.Errorf("Expected empty ServiceID, got %s", cfg.ServiceID)
580576
}
@@ -671,3 +667,66 @@ func TestConfigCommands_Integration(t *testing.T) {
671667
t.Errorf("Expected output reset to default %s, got %s", config.DefaultOutput, cfg.Output)
672668
}
673669
}
670+
671+
func TestConfigSet_OutputDoesPersist(t *testing.T) {
672+
tmpDir, _ := setupConfigTest(t)
673+
674+
// Start with default config (no output setting in file)
675+
configFile := config.GetConfigFile(tmpDir)
676+
677+
// Execute config set to explicitly set output to json
678+
_, err := executeConfigCommand("config", "set", "output", "json")
679+
if err != nil {
680+
t.Fatalf("Failed to set output to json: %v", err)
681+
}
682+
683+
// Read the config file directly
684+
configBytes, err := os.ReadFile(configFile)
685+
if err != nil {
686+
t.Fatalf("Failed to read config file: %v", err)
687+
}
688+
689+
// Parse the YAML to check
690+
var configMap map[string]interface{}
691+
if err := yaml.Unmarshal(configBytes, &configMap); err != nil {
692+
t.Fatalf("Failed to parse config YAML: %v", err)
693+
}
694+
695+
if outputVal, exists := configMap["output"]; !exists || outputVal != "json" {
696+
t.Errorf("Expected output in config file to be 'json', got: %v (exists: %v)", outputVal, exists)
697+
}
698+
699+
// Also verify by loading config
700+
cfg, err := config.Load()
701+
if err != nil {
702+
t.Fatalf("Failed to load config: %v", err)
703+
}
704+
705+
if cfg.Output != "json" {
706+
t.Errorf("Expected loaded config output to be 'json', got: %s", cfg.Output)
707+
}
708+
709+
// Now test that setting it to a different value updates the file
710+
_, err = executeConfigCommand("config", "set", "output", "yaml")
711+
if err != nil {
712+
t.Fatalf("Failed to set output to yaml: %v", err)
713+
}
714+
715+
// Read the config file again
716+
configBytes, err = os.ReadFile(configFile)
717+
if err != nil {
718+
t.Fatalf("Failed to read config file after second set: %v", err)
719+
}
720+
721+
configContent := string(configBytes)
722+
723+
// Verify that output was updated in the config file
724+
if !strings.Contains(configContent, "output: yaml") {
725+
t.Errorf("Config file should contain 'output: yaml' after update. Config content:\n%s", configContent)
726+
}
727+
728+
// Should NOT contain the old value
729+
if strings.Contains(configContent, "output: json") {
730+
t.Errorf("Config file should NOT contain old 'output: json' value. Config content:\n%s", configContent)
731+
}
732+
}

‎internal/tiger/cmd/db_test.go‎

Lines changed: 37 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,10 @@ func setupDBTest(t *testing.T) string {
3232

3333
// Reset global config and viper to ensure test isolation
3434
config.ResetGlobalConfig()
35-
viper.Reset()
3635

3736
t.Cleanup(func() {
3837
// Reset global config and viper first
3938
config.ResetGlobalConfig()
40-
viper.Reset()
4139
// Clean up environment variable BEFORE cleaning up file system
4240
os.Unsetenv("TIGER_CONFIG_DIR")
4341
// Then clean up file system
@@ -64,12 +62,10 @@ func TestDBConnectionString_NoServiceID(t *testing.T) {
6462
tmpDir := setupDBTest(t)
6563

6664
// Set up config with project ID but no default service ID
67-
cfg := &config.Config{
68-
APIURL: "https://api.tigerdata.com/public/v1",
69-
ProjectID: "test-project-123",
70-
ConfigDir: tmpDir,
71-
}
72-
err := cfg.Save()
65+
_, err := config.UseTestConfig(tmpDir, map[string]any{
66+
"api_url": "https://api.tigerdata.com/public/v1",
67+
"project_id": "test-project-123",
68+
})
7369
if err != nil {
7470
t.Fatalf("Failed to save test config: %v", err)
7571
}
@@ -96,13 +92,11 @@ func TestDBConnectionString_NoAuth(t *testing.T) {
9692
tmpDir := setupDBTest(t)
9793

9894
// Set up config with project ID and service ID
99-
cfg := &config.Config{
100-
APIURL: "https://api.tigerdata.com/public/v1",
101-
ProjectID: "test-project-123",
102-
ServiceID: "svc-12345",
103-
ConfigDir: tmpDir,
104-
}
105-
err := cfg.Save()
95+
_, err := config.UseTestConfig(tmpDir, map[string]any{
96+
"api_url": "https://api.tigerdata.com/public/v1",
97+
"project_id": "test-project-123",
98+
"service_id": "svc-12345",
99+
})
106100
if err != nil {
107101
t.Fatalf("Failed to save test config: %v", err)
108102
}
@@ -172,12 +166,10 @@ func TestDBConnect_NoServiceID(t *testing.T) {
172166
tmpDir := setupDBTest(t)
173167

174168
// Set up config with project ID but no default service ID
175-
cfg := &config.Config{
176-
APIURL: "https://api.tigerdata.com/public/v1",
177-
ProjectID: "test-project-123",
178-
ConfigDir: tmpDir,
179-
}
180-
err := cfg.Save()
169+
_, err := config.UseTestConfig(tmpDir, map[string]any{
170+
"api_url": "https://api.tigerdata.com/public/v1",
171+
"project_id": "test-project-123",
172+
})
181173
if err != nil {
182174
t.Fatalf("Failed to save test config: %v", err)
183175
}
@@ -204,13 +196,11 @@ func TestDBConnect_NoAuth(t *testing.T) {
204196
tmpDir := setupDBTest(t)
205197

206198
// Set up config with project ID and service ID
207-
cfg := &config.Config{
208-
APIURL: "https://api.tigerdata.com/public/v1",
209-
ProjectID: "test-project-123",
210-
ServiceID: "svc-12345",
211-
ConfigDir: tmpDir,
212-
}
213-
err := cfg.Save()
199+
_, err := config.UseTestConfig(tmpDir, map[string]any{
200+
"api_url": "https://api.tigerdata.com/public/v1",
201+
"project_id": "test-project-123",
202+
"service_id": "svc-12345",
203+
})
214204
if err != nil {
215205
t.Fatalf("Failed to save test config: %v", err)
216206
}
@@ -237,13 +227,11 @@ func TestDBConnect_PsqlNotFound(t *testing.T) {
237227
tmpDir := setupDBTest(t)
238228

239229
// Set up config
240-
cfg := &config.Config{
241-
APIURL: "http://localhost:9999",
242-
ProjectID: "test-project-123",
243-
ServiceID: "svc-12345",
244-
ConfigDir: tmpDir,
245-
}
246-
err := cfg.Save()
230+
_, err := config.UseTestConfig(tmpDir, map[string]any{
231+
"api_url": "http://localhost:9999",
232+
"project_id": "test-project-123",
233+
"service_id": "svc-12345",
234+
})
247235
if err != nil {
248236
t.Fatalf("Failed to save test config: %v", err)
249237
}
@@ -596,12 +584,10 @@ func TestDBTestConnection_NoServiceID(t *testing.T) {
596584
tmpDir := setupDBTest(t)
597585

598586
// Set up config with project ID but no default service ID
599-
cfg := &config.Config{
600-
APIURL: "https://api.tigerdata.com/public/v1",
601-
ProjectID: "test-project-123",
602-
ConfigDir: tmpDir,
603-
}
604-
err := cfg.Save()
587+
_, err := config.UseTestConfig(tmpDir, map[string]any{
588+
"api_url": "https://api.tigerdata.com/public/v1",
589+
"project_id": "test-project-123",
590+
})
605591
if err != nil {
606592
t.Fatalf("Failed to save test config: %v", err)
607593
}
@@ -628,13 +614,11 @@ func TestDBTestConnection_NoAuth(t *testing.T) {
628614
tmpDir := setupDBTest(t)
629615

630616
// Set up config with project ID and service ID
631-
cfg := &config.Config{
632-
APIURL: "https://api.tigerdata.com/public/v1",
633-
ProjectID: "test-project-123",
634-
ServiceID: "svc-12345",
635-
ConfigDir: tmpDir,
636-
}
637-
err := cfg.Save()
617+
_, err := config.UseTestConfig(tmpDir, map[string]any{
618+
"api_url": "https://api.tigerdata.com/public/v1",
619+
"project_id": "test-project-123",
620+
"service_id": "svc-12345",
621+
})
638622
if err != nil {
639623
t.Fatalf("Failed to save test config: %v", err)
640624
}
@@ -980,13 +964,11 @@ func TestDBTestConnection_TimeoutParsing(t *testing.T) {
980964
tmpDir := setupDBTest(t)
981965

982966
// Set up config
983-
cfg := &config.Config{
984-
APIURL: "http://localhost:9999", // Non-existent server
985-
ProjectID: "test-project-123",
986-
ServiceID: "svc-12345",
987-
ConfigDir: tmpDir,
988-
}
989-
err := cfg.Save()
967+
_, err := config.UseTestConfig(tmpDir, map[string]any{
968+
"api_url": "http://localhost:9999", // Non-existent server
969+
"project_id": "test-project-123",
970+
"service_id": "svc-12345",
971+
})
990972
if err != nil {
991973
t.Fatalf("Failed to save test config: %v", err)
992974
}

‎internal/tiger/cmd/integration_test.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ func setupIntegrationTest(t *testing.T) string {
3030

3131
// Reset global config and viper to ensure test isolation
3232
config.ResetGlobalConfig()
33-
viper.Reset()
3433

3534
// Re-establish viper environment configuration after reset
3635
viper.SetEnvPrefix("TIGER")
@@ -50,7 +49,6 @@ func setupIntegrationTest(t *testing.T) string {
5049
t.Cleanup(func() {
5150
// Reset global config and viper first
5251
config.ResetGlobalConfig()
53-
viper.Reset()
5452
// Clean up environment variable BEFORE cleaning up file system
5553
os.Unsetenv("TIGER_CONFIG_DIR")
5654
// Then clean up file system
@@ -65,7 +63,6 @@ func executeIntegrationCommand(args ...string) (string, error) {
6563
// Reset both global config and viper before each command execution
6664
// This ensures fresh config loading with proper flag precedence
6765
config.ResetGlobalConfig()
68-
viper.Reset()
6966

7067
// Re-establish viper environment configuration after reset
7168
viper.SetEnvPrefix("TIGER")

‎internal/tiger/cmd/root_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import (
1010

1111
func TestMain(m *testing.M) {
1212
// Clean up any global state before tests
13-
viper.Reset()
13+
config.ResetGlobalConfig()
1414
code := m.Run()
1515
os.Exit(code)
1616
}
@@ -27,7 +27,7 @@ func setupTestCommand(t *testing.T) (string, func()) {
2727
// Clean up function
2828
cleanup := func() {
2929
os.RemoveAll(tmpDir)
30-
viper.Reset()
30+
config.ResetGlobalConfig()
3131
}
3232

3333
t.Cleanup(cleanup)
@@ -121,7 +121,7 @@ func TestFlagBindingWithViper(t *testing.T) {
121121
}
122122

123123
// Reset for next test
124-
viper.Reset()
124+
config.ResetGlobalConfig()
125125

126126
// Test 2: Flag should override environment variable
127127
testCmd2 := buildRootCmd()

‎internal/tiger/cmd/service.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -908,8 +908,7 @@ func setDefaultService(serviceID string, output io.Writer) error {
908908
return fmt.Errorf("failed to load config: %w", err)
909909
}
910910

911-
cfg.ServiceID = serviceID
912-
if err := cfg.Save(); err != nil {
911+
if err := cfg.Set("service_id", serviceID); err != nil {
913912
return fmt.Errorf("failed to save config: %w", err)
914913
}
915914

0 commit comments

Comments
 (0)