From 03efa19b232da07540ff6e6ced53050c62f01537 Mon Sep 17 00:00:00 2001 From: stenh0use Date: Sat, 2 May 2026 16:35:25 -0400 Subject: [PATCH] fix CLI missing-cluster and stop failure behavior Enforce not-found errors for missing cluster config in get/rm flows and return errors when stop has failed containers, with aligned cluster tests for persisted-config contract. Co-Authored-By: Claude Opus 4.7 --- pkg/cluster/manager.go | 22 +++--- pkg/cluster/manager_behavior_test.go | 40 +++++++++- pkg/cluster/manager_get_test.go | 113 +++++++++++++++++++++------ pkg/cluster/manager_wait_test.go | 40 +++++++--- pkg/cluster/stop_test.go | 24 +++++- pkg/cmd/hind/rm/rm_test.go | 26 +++++- pkg/cmd/hind/stop/stop.go | 8 +- pkg/cmd/hind/stop/stop_test.go | 16 +++- 8 files changed, 231 insertions(+), 58 deletions(-) diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index 3f91c85..4f7a338 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -220,15 +220,16 @@ func (m *Manager) StopWithOptions(ctx context.Context, opts StopOptions) (StopRe func (m *Manager) Delete(ctx context.Context) error { existed := m.ConfigFileExists() + if !existed { + return fmt.Errorf("cluster '%s' not found", m.config.Name) + } - if existed { - // Load existing config, else just use the config created by New() - cfg, err := m.loadConfig() - if err != nil { - return fmt.Errorf("failed to load cluster config: %w", err) - } - m.config = cfg + // Load existing config, else just use the config created by New() + cfg, err := m.loadConfig() + if err != nil { + return fmt.Errorf("failed to load cluster config: %w", err) } + m.config = cfg // TODO: make delete idempotent @@ -321,13 +322,12 @@ func (m *Manager) ConfigFileExists() bool { } // LoadPersistedConfig loads cluster configuration from disk when available. -// If no persisted config exists, the current in-memory config is left unchanged. func (m *Manager) LoadPersistedConfig() error { if !m.ConfigFileExists() { - if m.config == nil || m.config.Name == "" { - return fmt.Errorf("cluster config not found") + if m.config != nil && m.config.Name != "" { + return fmt.Errorf("cluster '%s' not found", m.config.Name) } - return nil + return fmt.Errorf("cluster config not found") } cfg, err := m.loadConfig() diff --git a/pkg/cluster/manager_behavior_test.go b/pkg/cluster/manager_behavior_test.go index ed0aa1a..d182040 100644 --- a/pkg/cluster/manager_behavior_test.go +++ b/pkg/cluster/manager_behavior_test.go @@ -27,13 +27,25 @@ func newManagerForBehaviorTests(t *testing.T, clusterName string, cfg *config.Cl t.Fatalf("file.New() error = %v", err) } - return &Manager{ + m := &Manager{ logger: &log.Logger{Handler: discard.New(), Level: log.ErrorLevel}, provider: stub, config: cfg, fm: fm, configFile: file.JoinPath(ClusterConfigDir, clusterName, ClusterConfigFile), } + + if cfg != nil && cfg.Name != "" { + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + if err := m.fm.WriteFile(m.configFile, data); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + } + + return m } func TestManagerStart_ReturnsErrorWhenPersistedConfigInvalid(t *testing.T) { @@ -112,14 +124,17 @@ func TestManagerStart_UsesPersistedConfigForReconcile(t *testing.T) { func TestManagerGet_ReturnsErrorWhenNoPersistedConfigAndNoDefaults(t *testing.T) { t.Parallel() - m := newManagerForBehaviorTests(t, "demo", &config.Cluster{}, &mock.ClientStub{}) + m := newManagerForBehaviorTests(t, "demo", &config.Cluster{Name: "demo"}, &mock.ClientStub{}) + if err := m.fm.RemoveFile(m.configFile); err != nil { + t.Fatalf("RemoveFile() error = %v", err) + } _, err := m.Get(context.Background()) if err == nil { t.Fatal("Get() expected error, got nil") } - if !strings.Contains(err.Error(), "cluster config not found") { - t.Fatalf("Get() error = %q, want missing-config error", err) + if !strings.Contains(err.Error(), "cluster 'demo' not found") { + t.Fatalf("Get() error = %q, want missing-cluster error", err) } } @@ -156,6 +171,23 @@ func TestManagerStop_AggregatesStopContainerError(t *testing.T) { } } +func TestManagerDelete_ReturnsNotFoundWhenConfigMissing(t *testing.T) { + t.Parallel() + + m := newManagerForBehaviorTests(t, "ghost", &config.Cluster{Name: "ghost"}, &mock.ClientStub{}) + if err := m.fm.RemoveFile(m.configFile); err != nil { + t.Fatalf("RemoveFile() error = %v", err) + } + + err := m.Delete(context.Background()) + if err == nil { + t.Fatal("Delete() expected error, got nil") + } + if !strings.Contains(err.Error(), "cluster 'ghost' not found") { + t.Fatalf("Delete() error = %q, want missing-cluster error", err) + } +} + func TestManagerDelete_ReturnsWrappedStopContainerError(t *testing.T) { t.Parallel() diff --git a/pkg/cluster/manager_get_test.go b/pkg/cluster/manager_get_test.go index 5a4a88c..53a414b 100644 --- a/pkg/cluster/manager_get_test.go +++ b/pkg/cluster/manager_get_test.go @@ -80,19 +80,36 @@ func TestManagerGet_ReturnsInspectNetworkError(t *testing.T) { t.Parallel() wantErr := errors.New("docker daemon unavailable") + root := t.TempDir() + fm, err := file.New(root) + if err != nil { + t.Fatalf("file.New() error = %v", err) + } + cfg := &config.Cluster{ + Name: "demo", + Network: config.Network{Name: "hind.demo"}, + } + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + configPath := file.JoinPath(ClusterConfigDir, "demo", ClusterConfigFile) + if err := fm.WriteFile(configPath, data); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + m := &Manager{ provider: &mock.ClientStub{ InspectNetworkFn: func(ctx context.Context, name string) (*provider.NetworkInfo, error) { return nil, wantErr }, }, - config: &config.Cluster{ - Name: "demo", - Network: config.Network{Name: "hind.demo"}, - }, + config: cfg, + fm: fm, + configFile: configPath, } - _, err := m.Get(context.Background()) + _, err = m.Get(context.Background()) if err == nil { t.Fatal("Get() expected error, got nil") } @@ -108,6 +125,25 @@ func TestManagerGet_ReturnsInspectContainerError(t *testing.T) { t.Parallel() wantErr := errors.New("inspect container failed") + root := t.TempDir() + fm, err := file.New(root) + if err != nil { + t.Fatalf("file.New() error = %v", err) + } + cfg := &config.Cluster{ + Name: "demo", + Network: config.Network{Name: "hind.demo"}, + Nodes: []config.Node{{Name: "hind.demo.consul.01"}}, + } + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + configPath := file.JoinPath(ClusterConfigDir, "demo", ClusterConfigFile) + if err := fm.WriteFile(configPath, data); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + m := &Manager{ provider: &mock.ClientStub{ InspectNetworkFn: func(ctx context.Context, name string) (*provider.NetworkInfo, error) { @@ -117,14 +153,12 @@ func TestManagerGet_ReturnsInspectContainerError(t *testing.T) { return nil, wantErr }, }, - config: &config.Cluster{ - Name: "demo", - Network: config.Network{Name: "hind.demo"}, - Nodes: []config.Node{{Name: "hind.demo.consul.01"}}, - }, + config: cfg, + fm: fm, + configFile: configPath, } - _, err := m.Get(context.Background()) + _, err = m.Get(context.Background()) if err == nil { t.Fatal("Get() expected error, got nil") } @@ -277,7 +311,7 @@ func TestManagerStop_UsesPersistedTopology(t *testing.T) { } } -func TestManagerLoadPersistedConfig_MissingFileKeepsDefaults(t *testing.T) { +func TestManagerLoadPersistedConfig_MissingFileReturnsNotFound(t *testing.T) { t.Parallel() m := &Manager{ @@ -288,12 +322,12 @@ func TestManagerLoadPersistedConfig_MissingFileKeepsDefaults(t *testing.T) { }, } - if err := m.LoadPersistedConfig(); err != nil { - t.Fatalf("LoadPersistedConfig() unexpected error: %v", err) + err := m.LoadPersistedConfig() + if err == nil { + t.Fatal("LoadPersistedConfig() expected error when persisted config file is missing") } - - if m.config.Network.Name != "hind.demo-default" { - t.Fatalf("LoadPersistedConfig() changed defaults unexpectedly; got network %q", m.config.Network.Name) + if !strings.Contains(err.Error(), "cluster 'demo' not found") { + t.Fatalf("LoadPersistedConfig() error = %q, want missing-cluster error", err) } } @@ -310,6 +344,25 @@ func TestManagerStop_PropagatesInspectContainerError(t *testing.T) { t.Parallel() wantErr := errors.New("container inspect failed") + root := t.TempDir() + fm, err := file.New(root) + if err != nil { + t.Fatalf("file.New() error = %v", err) + } + cfg := &config.Cluster{ + Name: "demo", + Network: config.Network{Name: "hind.demo"}, + Nodes: []config.Node{{Name: "hind.demo.consul.01"}}, + } + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + configPath := file.JoinPath(ClusterConfigDir, "demo", ClusterConfigFile) + if err := fm.WriteFile(configPath, data); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + m := &Manager{ logger: &log.Logger{Handler: discard.New(), Level: log.ErrorLevel}, provider: &mock.ClientStub{ @@ -318,14 +371,12 @@ func TestManagerStop_PropagatesInspectContainerError(t *testing.T) { return nil, wantErr }, }, - config: &config.Cluster{ - Name: "demo", - Network: config.Network{Name: "hind.demo"}, - Nodes: []config.Node{{Name: "hind.demo.consul.01"}}, - }, + config: cfg, + fm: fm, + configFile: configPath, } - err := m.Stop(context.Background()) + err = m.Stop(context.Background()) if err == nil { t.Fatal("Stop() expected error when InspectContainer returns error, got nil") } @@ -361,6 +412,14 @@ func TestManagerDelete_PropagatesInspectContainerError(t *testing.T) { configFile: file.JoinPath(ClusterConfigDir, "demo", ClusterConfigFile), } + data, err := json.Marshal(m.config) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + if err := fm.WriteFile(m.configFile, data); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + err = m.Delete(context.Background()) if err == nil { t.Fatal("Delete() expected error when InspectContainer returns error, got nil") @@ -401,6 +460,14 @@ func TestManagerDelete_PropagatesInspectNetworkError(t *testing.T) { configFile: file.JoinPath(ClusterConfigDir, "demo", ClusterConfigFile), } + data, err := json.Marshal(m.config) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + if err := fm.WriteFile(m.configFile, data); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + err = m.Delete(context.Background()) if err == nil { t.Fatal("Delete() expected error when InspectNetwork returns error, got nil") diff --git a/pkg/cluster/manager_wait_test.go b/pkg/cluster/manager_wait_test.go index bd1b836..d7f8bc9 100644 --- a/pkg/cluster/manager_wait_test.go +++ b/pkg/cluster/manager_wait_test.go @@ -2,6 +2,7 @@ package cluster import ( "context" + "encoding/json" "errors" "testing" "time" @@ -9,11 +10,28 @@ import ( "github.com/apex/log" "github.com/apex/log/handlers/discard" "github.com/stenh0use/hind/pkg/config" + "github.com/stenh0use/hind/pkg/file" "github.com/stenh0use/hind/pkg/provider" "github.com/stenh0use/hind/pkg/provider/mock" ) func TestWaitForContainersRunning_ReturnsContextErrorPromptly(t *testing.T) { + root := t.TempDir() + fm, err := file.New(root) + if err != nil { + t.Fatalf("file.New() error = %v", err) + } + + clusterCfg := &config.Cluster{ + Name: "test", + Network: config.Network{ + Name: "hind.test", + }, + Nodes: []config.Node{ + {Name: "hind.test.consul.01"}, + }, + } + m := &Manager{ logger: &log.Logger{Handler: discard.New()}, provider: &mock.ClientStub{ @@ -24,22 +42,24 @@ func TestWaitForContainersRunning_ReturnsContextErrorPromptly(t *testing.T) { return &provider.NetworkInfo{Name: name}, nil }, }, - config: &config.Cluster{ - Name: "test", - Network: config.Network{ - Name: "hind.test", - }, - Nodes: []config.Node{ - {Name: "hind.test.consul.01"}, - }, - }, + config: clusterCfg, + fm: fm, + configFile: file.JoinPath(ClusterConfigDir, "test", ClusterConfigFile), + } + + data, err := json.Marshal(clusterCfg) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + if err := fm.WriteFile(m.configFile, data); err != nil { + t.Fatalf("WriteFile() error = %v", err) } ctx, cancel := context.WithCancel(context.Background()) cancel() start := time.Now() - err := m.waitForContainersRunning(ctx, 5*time.Second) + err = m.waitForContainersRunning(ctx, 5*time.Second) elapsed := time.Since(start) if !errors.Is(err, context.Canceled) { diff --git a/pkg/cluster/stop_test.go b/pkg/cluster/stop_test.go index 2a64272..ac583c6 100644 --- a/pkg/cluster/stop_test.go +++ b/pkg/cluster/stop_test.go @@ -2,6 +2,7 @@ package cluster import ( "context" + "encoding/json" "errors" "testing" @@ -9,6 +10,7 @@ import ( "github.com/apex/log/handlers/discard" "github.com/stenh0use/hind/pkg/config" + "github.com/stenh0use/hind/pkg/file" "github.com/stenh0use/hind/pkg/provider" "github.com/stenh0use/hind/pkg/provider/mock" ) @@ -55,10 +57,26 @@ func TestStopWithOptions(t *testing.T) { return nil } + root := t.TempDir() + fm, err := file.New(root) + if err != nil { + t.Fatalf("file.New() error = %v", err) + } + cfg := &config.Cluster{Name: tt.name, Nodes: []config.Node{{Name: "n1"}, {Name: "n2"}}} + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + configPath := file.JoinPath(ClusterConfigDir, tt.name, ClusterConfigFile) + if err := fm.WriteFile(configPath, data); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } m := &Manager{ - logger: &log.Logger{Handler: discard.New(), Level: log.ErrorLevel}, - provider: client, - config: &config.Cluster{Name: tt.name, Nodes: []config.Node{{Name: "n1"}, {Name: "n2"}}}, + logger: &log.Logger{Handler: discard.New(), Level: log.ErrorLevel}, + provider: client, + config: cfg, + fm: fm, + configFile: configPath, } res, err := m.StopWithOptions(context.Background(), StopOptions{Force: tt.force}) diff --git a/pkg/cmd/hind/rm/rm_test.go b/pkg/cmd/hind/rm/rm_test.go index c49d833..13fd557 100644 --- a/pkg/cmd/hind/rm/rm_test.go +++ b/pkg/cmd/hind/rm/rm_test.go @@ -4,6 +4,7 @@ import ( "context" "io" "os" + "strings" "testing" "time" @@ -71,13 +72,34 @@ func TestCommandFlags(t *testing.T) { } // stubDeleter is a no-op clusterDeleter used to bypass real Docker calls in tests. -type stubDeleter struct{} +type stubDeleter struct { + deleteErr error +} -func (s *stubDeleter) Delete(_ context.Context) error { return nil } +func (s *stubDeleter) Delete(_ context.Context) error { return s.deleteErr } // TestRunE_ClearsActiveClusterOnDelete verifies that when the cluster being removed // is the currently active cluster, runE calls ClearActiveCluster so that subsequent // commands fall back to the "default" cluster resolution path. +func TestRunE_ReturnsErrorWhenDeleteFails(t *testing.T) { + orig := newClusterManagerFn + newClusterManagerFn = func(_ *log.Logger, _ string) (clusterDeleter, error) { + return &stubDeleter{deleteErr: context.DeadlineExceeded}, nil + } + defer func() { newClusterManagerFn = orig }() + + logger := &log.Logger{Handler: discard.New(), Level: log.ErrorLevel} + streams := cmd.IOStreams{Out: io.Discard, ErrOut: io.Discard} + + err := runE(context.Background(), logger, streams, DefaultDeleteTimeout, "ghost") + if err == nil { + t.Fatal("runE expected error for missing cluster delete") + } + if !strings.Contains(err.Error(), "failed to delete cluster") { + t.Fatalf("runE error = %v", err) + } +} + func TestRunE_ClearsActiveClusterOnDelete(t *testing.T) { // Redirect HOME so cluster state is isolated to this test. tmpDir := t.TempDir() diff --git a/pkg/cmd/hind/stop/stop.go b/pkg/cmd/hind/stop/stop.go index 95024e5..ddef549 100644 --- a/pkg/cmd/hind/stop/stop.go +++ b/pkg/cmd/hind/stop/stop.go @@ -112,12 +112,12 @@ func runE(ctx context.Context, logger *log.Logger, streams cmd.IOStreams, timeou fmt.Fprintf(streams.ErrOut, "Cluster '%s' is already stopped\n", clusterName) return nil } - if force { - fmt.Fprintf(streams.ErrOut, "Cluster '%s' force stopped\n", clusterName) - return nil - } if result.FailedCount > 0 { fmt.Fprintf(streams.ErrOut, "Cluster '%s' partially stopped\n", clusterName) + return fmt.Errorf("failed to stop %d container(s)", result.FailedCount) + } + if force { + fmt.Fprintf(streams.ErrOut, "Cluster '%s' force stopped\n", clusterName) return nil } diff --git a/pkg/cmd/hind/stop/stop_test.go b/pkg/cmd/hind/stop/stop_test.go index d185fea..980ab93 100644 --- a/pkg/cmd/hind/stop/stop_test.go +++ b/pkg/cmd/hind/stop/stop_test.go @@ -79,6 +79,13 @@ func TestRunEMessageContracts(t *testing.T) { result: cluster.StopResult{StoppedCount: 1, FailedCount: 1, Failures: []string{"n2"}}, wantLines: []string{"Failed to stop container 'n2'", "Cluster 'default' partially stopped"}, }, + { + name: "force with partial failure still errors", + force: true, + result: cluster.StopResult{StoppedCount: 1, FailedCount: 1, Failures: []string{"n2"}}, + wantLines: []string{"Failed to stop container 'n2'", "Cluster 'default' partially stopped"}, + wantForceOpt: true, + }, { name: "unhealthy pre-failed", result: cluster.StopResult{AlreadyStoppedCount: 1, FailedPreStopCount: 1}, @@ -109,7 +116,14 @@ func TestRunEMessageContracts(t *testing.T) { streams := cmd.IOStreams{Out: io.Discard, ErrOut: errBuf} err := runE(context.Background(), testLogger(), streams, DefaultStopTimeout, tt.force, tt.verbose, "") - if err != nil { + if tt.name == "partial failure warnings" || tt.name == "force with partial failure still errors" { + if err == nil { + t.Fatal("runE() expected error for partial failure") + } + if !strings.Contains(err.Error(), "failed to stop 1 container(s)") { + t.Fatalf("runE() error = %v", err) + } + } else if err != nil { t.Fatalf("runE() error = %v", err) }