feat(cluster): health/test-connection/utilization commands#83
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new stackctl cluster subcommands to surface cluster connectivity and health/utilization endpoints from the backend, and aligns the CLI’s health summary type with the actual API payload to avoid silently rendering zero values.
Changes:
- Added
cluster test-connection,cluster health,cluster nodes,cluster namespaces, andcluster utilizationcommands with table/json/yaml/quiet output support. - Reworked
types.ClusterHealthSummaryto match the backend JSON shape and updated cluster rendering accordingly (including derivedhealthy/degraded/unknown). - Extended unit, integration, and e2e test coverage for the new client methods, command output modes, and mock server routes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/cmd/cluster.go | Adds new cluster subcommands and updates cluster health rendering/derivation. |
| cli/pkg/client/client.go | Adds client methods for the new cluster endpoints. |
| cli/pkg/types/types.go | Updates ClusterHealthSummary and adds new API-shaped types for nodes/namespaces/utilization/test. |
| cli/cmd/cluster_test.go | Adds command-level tests for new subcommands across output modes and status derivation branches. |
| cli/pkg/client/client_test.go | Adds unit tests validating method/URL/verb behavior for new client calls. |
| cli/test/integration/cluster_integration_test.go | Extends the integration mock server and adds workflow + cobra health integration tests. |
| cli/test/e2e/cli_e2e_test.go | Extends the e2e mock server routes and adds an e2e cluster health test covering table/json/quiet. |
Comment on lines
+395
to
+399
| // printCluster renders a single cluster in the configured output format. | ||
| // deriveHealthStatus collapses the cluster health summary into a single status | ||
| // label suitable for the table/quiet output modes. `unknown` covers the case | ||
| // where no nodes were reported (registry hasn't seen any, or the cluster is | ||
| // brand new) — distinct from `degraded`, which implies known-but-bad. |
Comment on lines
+1053
to
+1061
| // TestClusterConnection asks the backend to verify connectivity to the | ||
| // cluster's API server. On a reachable cluster the backend responds 200 with | ||
| // status=="success"; on an unreachable cluster it returns a non-2xx response | ||
| // which surfaces here as a client.APIError. | ||
| func (c *Client) TestClusterConnection(id string) (*types.ClusterTestConnectionResult, error) { | ||
| var result types.ClusterTestConnectionResult | ||
| if err := c.Post(fmt.Sprintf("/api/v1/clusters/%s/test", id), nil, &result); err != nil { | ||
| return nil, err | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in 530b42c — Client.do() now decodes both {error} and {message} body shapes (prefers error, falls back to message), so the backend's "Cluster is unreachable" reaches APIError.Message and surfaces in the user-facing 5xx rendering. TestTestClusterConnection_Unreachable now mocks the real {status,message} shape and asserts both layers.
Wires five new subcommands around already-shipped backend endpoints so DevOps users can do connectivity checks and capacity inspection from the CLI without curl/jq: stackctl cluster test-connection <id> POST /clusters/:id/test stackctl cluster health <id> GET /clusters/:id/health/summary stackctl cluster nodes <id> GET /clusters/:id/health/nodes stackctl cluster namespaces <id> GET /clusters/:id/namespaces stackctl cluster utilization <id> GET /clusters/:id/utilization The existing `types.ClusterHealthSummary` had stale field names (`Status`, `CPUUsage`, `MemUsage`, `CPUTotal`, `MemTotal`) that did not match the backend's `k8s.ClusterSummary` JSON shape, so `stackctl cluster get` was silently rendering zero values for most health fields. The type is rewritten to match the wire format (`node_count`, `ready_node_count`, `total_cpu`, `total_memory`, `allocatable_cpu`, `allocatable_memory`, `namespace_count`) and the `cluster get` rendering is updated to use the new fields. Out-of-tree consumers of `cli/pkg/types` should treat this as a breaking change. A `deriveHealthStatus` helper collapses the summary into one label — `healthy` when all nodes ready, `degraded` when some aren't, `unknown` when the cluster has zero nodes (registry unaware yet). Tests: unit (cmd + client), integration (Cobra in-process against httptest mock), and e2e (built binary against httptest mock) all cover the new verbs. Closes #65 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…format
- Split the combined comment block so deriveHealthStatus has its own
docstring above it, and printCluster gets its own leading comment.
Previously the printCluster comment appeared above deriveHealthStatus,
leaving printCluster undocumented.
- Change TestTestClusterConnection_Unreachable mock from non-standard
{status,message} payload to types.ErrorResponse{Error:...}, matching
the backend's actual gin.H{"error":...} format. Add assertion that the
error message is surfaced to the caller via APIError.UserFacingError().
omattsson
force-pushed
the
work/65-cluster-health
branch
from
ed251cc to
2e49b94
Compare
| json.NewEncoder(w).Encode(types.ErrorResponse{Error: "cluster not found"}) | ||
| return | ||
| } | ||
| if state.unreachable[id] { |
Comment on lines
+1053
to
+1062
| // TestClusterConnection asks the backend to verify connectivity to the | ||
| // cluster's API server. On a reachable cluster the backend responds 200 with | ||
| // status=="success"; on an unreachable cluster it returns a non-2xx response | ||
| // which surfaces here as a client.APIError. | ||
| func (c *Client) TestClusterConnection(id string) (*types.ClusterTestConnectionResult, error) { | ||
| var result types.ClusterTestConnectionResult | ||
| if err := c.Post(fmt.Sprintf("/api/v1/clusters/%s/test", id), nil, &result); err != nil { | ||
| return nil, err | ||
| } | ||
| return &resp, nil | ||
| return &result, nil |
Owner
Author
There was a problem hiding this comment.
Fixed in 530b42c — decoder now tries error first and falls back to message (with a final fallback to http.StatusText if both are empty), so the backend-provided context surfaces in APIError.Message instead of getting dropped.
Comment on lines
+2157
to
+2172
| func TestTestClusterConnection_Unreachable(t *testing.T) { | ||
| t.Parallel() | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusBadGateway) | ||
| json.NewEncoder(w).Encode(types.ErrorResponse{Error: "Cluster is unreachable"}) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| c := New(server.URL) | ||
| result, err := c.TestClusterConnection("1") | ||
| require.Error(t, err) | ||
| assert.Nil(t, result) | ||
| var apiErr *APIError | ||
| require.ErrorAs(t, err, &apiErr) | ||
| assert.Contains(t, apiErr.UserFacingError(), "Cluster is unreachable") | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in 530b42c — mock now returns the real wire shape ({"status":"error","message":"Cluster is unreachable"}) and the test asserts apiErr.Message == "Cluster is unreachable" plus that UserFacingError() includes both "Server error" and the backend message. Catches regressions in either the decoder or the rendering layer.
Comment on lines
722
to
725
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, resp.Status) | ||
| fmt.Fprintln(printer.Writer, result.Status) | ||
| return nil | ||
| } |
Comment on lines
762
to
765
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, id) | ||
| fmt.Fprintln(printer.Writer, deriveHealthStatus(health)) | ||
| return nil | ||
| } |
Comment on lines
806
to
812
| if printer.Quiet { | ||
| for _, n := range nodes { | ||
| fmt.Fprintln(printer.Writer, n.Name) | ||
| names := make([]string, len(nodes)) | ||
| for i, n := range nodes { | ||
| names[i] = n.Name | ||
| } | ||
| printer.PrintIDs(names) | ||
| return nil |
Comment on lines
857
to
864
| if printer.Quiet { | ||
| for _, ns := range namespaces { | ||
| fmt.Fprintln(printer.Writer, ns.Name) | ||
| names := make([]string, len(namespaces)) | ||
| for i, n := range namespaces { | ||
| names[i] = n.Name | ||
| } | ||
| printer.PrintIDs(names) | ||
| return nil | ||
| } |
Comment on lines
906
to
912
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, id) | ||
| names := make([]string, len(util.Namespaces)) | ||
| for i, ns := range util.Namespaces { | ||
| names[i] = ns.Namespace | ||
| } | ||
| printer.PrintIDs(names) | ||
| return nil |
- Remove quiet mode from test-connection, health, nodes, namespaces,
utilization: none produce numeric IDs per commands.instructions rule
- Update affected tests to assert table output is produced when
printer.Quiet is set (quiet is silently ignored)
- Fix data race in cluster integration test: read state.unreachable
under state.mu to avoid concurrent map access
- Standardise 502 mock response to types.ErrorResponse{} in both
cluster_test.go and cluster_integration_test.go (matches real
backend gin.H{"error":...} format)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
cli/cmd/cluster.go:815
--quietis currently ignored forcluster nodes. If quiet mode is meant to be supported (per PR description), it needs an explicitif printer.Quiet { ... }branch here (for example, printing node names one per line). Right now the command always prints table/JSON/YAML output.
nodes, err := c.GetClusterNodes(id)
if err != nil {
return err
}
switch printer.Format {
case output.FormatJSON:
return printer.PrintJSON(nodes)
case output.FormatYAML:
return printer.PrintYAML(nodes)
default:
headers := []string{"NAME", "STATUS", "PODS", "CPU", "MEMORY"}
rows := make([][]string, len(nodes))
for i, n := range nodes {
rows[i] = []string{
n.Name,
printer.StatusColor(n.Status),
strconv.Itoa(n.PodCount),
n.Allocatable.CPU,
n.Allocatable.Memory,
}
}
return printer.PrintTable(headers, rows)
}
},
cli/cmd/cluster.go:855
--quietis currently ignored forcluster namespaces. If quiet mode is meant to be supported, add an explicitprinter.Quietbranch (e.g., print namespace names one per line). The current implementation always renders table/JSON/YAML output regardless of--quiet.
namespaces, err := c.GetClusterNamespaces(id)
if err != nil {
return err
}
switch printer.Format {
case output.FormatJSON:
return printer.PrintJSON(namespaces)
case output.FormatYAML:
return printer.PrintYAML(namespaces)
default:
headers := []string{"NAME", "PHASE", "CREATED"}
rows := make([][]string, len(namespaces))
for i, n := range namespaces {
created := ""
if !n.CreatedAt.IsZero() {
created = n.CreatedAt.UTC().Format("2006-01-02 15:04:05")
}
rows[i] = []string{n.Name, printer.StatusColor(n.Phase), created}
}
return printer.PrintTable(headers, rows)
}
},
cli/cmd/cluster.go:902
--quietis currently ignored forcluster utilization. If quiet mode is intended to be supported, add aprinter.Quietbranch describing what the quiet output should be (e.g., cluster ID or namespace names). As written, the command always renders table/JSON/YAML output.
util, err := c.GetClusterUtilization(id)
if err != nil {
return err
}
switch printer.Format {
case output.FormatJSON:
return printer.PrintJSON(util)
case output.FormatYAML:
return printer.PrintYAML(util)
default:
headers := []string{"NAMESPACE", "CPU USED", "CPU LIMIT", "MEM USED", "MEM LIMIT", "PODS"}
rows := make([][]string, len(util.Namespaces))
for i, ns := range util.Namespaces {
podCol := strconv.Itoa(ns.PodCount)
if ns.PodLimit > 0 {
podCol = fmt.Sprintf("%d/%d", ns.PodCount, ns.PodLimit)
}
rows[i] = []string{
ns.Namespace,
ns.CPUUsed,
ns.CPULimit,
ns.MemoryUsed,
ns.MemoryLimit,
podCol,
}
}
return printer.PrintTable(headers, rows)
}
},
Comment on lines
+717
to
733
| result, err := c.TestClusterConnection(id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, resp.Status) | ||
| return nil | ||
| } | ||
|
|
||
| switch printer.Format { | ||
| case output.FormatJSON: | ||
| return printer.PrintJSON(resp) | ||
| return printer.PrintJSON(result) | ||
| case output.FormatYAML: | ||
| return printer.PrintYAML(resp) | ||
| return printer.PrintYAML(result) | ||
| default: | ||
| fields := []output.KeyValue{ | ||
| {Key: "Status", Value: printer.StatusColor(resp.Status)}, | ||
| {Key: "Message", Value: resp.Message}, | ||
| } | ||
| if resp.ServerVersion != "" { | ||
| fields = append(fields, output.KeyValue{Key: "Server Version", Value: resp.ServerVersion}) | ||
| } | ||
| return printer.PrintSingle(resp, fields) | ||
| return printer.PrintSingle(result, []output.KeyValue{ | ||
| {Key: "Status", Value: printer.StatusColor(result.Status)}, | ||
| {Key: "Message", Value: result.Message}, | ||
| {Key: "Server Version", Value: result.ServerVersion}, | ||
| }) | ||
| } |
Comment on lines
752
to
+771
| health, err := c.GetClusterHealth(id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, id) | ||
| return nil | ||
| } | ||
|
|
||
| switch printer.Format { | ||
| case output.FormatJSON: | ||
| return printer.PrintJSON(health) | ||
| case output.FormatYAML: | ||
| return printer.PrintYAML(health) | ||
| default: | ||
| readyStatus := "healthy" | ||
| if health.ReadyNodeCount < health.NodeCount { | ||
| readyStatus = "degraded" | ||
| } | ||
| fields := []output.KeyValue{ | ||
| {Key: "Status", Value: printer.StatusColor(readyStatus)}, | ||
| {Key: "Nodes", Value: strconv.Itoa(health.NodeCount)}, | ||
| {Key: "Ready Nodes", Value: strconv.Itoa(health.ReadyNodeCount)}, | ||
| {Key: "Total CPU", Value: health.TotalCPU}, | ||
| {Key: "Allocatable CPU", Value: health.AllocatableCPU}, | ||
| {Key: "Total Memory", Value: health.TotalMemory}, | ||
| {Key: "Allocatable Memory", Value: health.AllocatableMemory}, | ||
| return printer.PrintSingle(health, []output.KeyValue{ | ||
| {Key: "Health Status", Value: printer.StatusColor(deriveHealthStatus(health))}, | ||
| {Key: "Ready Nodes", Value: fmt.Sprintf("%d/%d", health.ReadyNodeCount, health.NodeCount)}, | ||
| {Key: "CPU Allocatable", Value: health.AllocatableCPU}, | ||
| {Key: "CPU Total", Value: health.TotalCPU}, | ||
| {Key: "Memory Allocatable", Value: health.AllocatableMemory}, | ||
| {Key: "Memory Total", Value: health.TotalMemory}, | ||
| {Key: "Namespaces", Value: strconv.Itoa(health.NamespaceCount)}, | ||
| } | ||
| return printer.PrintSingle(health, fields) | ||
| }) |
Comment on lines
+301
to
+307
| // ClusterTestConnectionResult is the response shape of | ||
| // POST /api/v1/clusters/:id/test. On success Status == "success"; on a | ||
| // reachable-but-erroring cluster the backend returns 502 with Status == "error". | ||
| type ClusterTestConnectionResult struct { | ||
| Status string `json:"status" yaml:"status"` | ||
| Message string `json:"message,omitempty" yaml:"message,omitempty"` | ||
| ServerVersion string `json:"server_version,omitempty" yaml:"server_version,omitempty"` |
Owner
Author
There was a problem hiding this comment.
Fixed in 530b42c — docstring rewritten. The struct is only populated on HTTP 200; on 502 the client decoder now extracts the backend message field into APIError.Message, which is what the doc now describes.
Comment on lines
1311
to
1327
| @@ -1366,47 +1321,34 @@ func TestClusterTestConnectionCmd_QuietOutput(t *testing.T) { | |||
|
|
|||
| err := clusterTestConnectionCmd.RunE(clusterTestConnectionCmd, []string{"1"}) | |||
| require.NoError(t, err) | |||
| assert.Equal(t, "ok\n", buf.String()) | |||
| } | |||
|
|
|||
| func TestClusterTestConnectionCmd_ParseIDError(t *testing.T) { | |||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| t.Fatal("should not reach server") | |||
| })) | |||
| defer server.Close() | |||
|
|
|||
| _ = setupClusterTestCmd(t, server.URL) | |||
|
|
|||
| err := clusterTestConnectionCmd.RunE(clusterTestConnectionCmd, []string{" "}) | |||
| require.Error(t, err) | |||
| assert.Contains(t, err.Error(), "must not be empty") | |||
| out := buf.String() | |||
| assert.Contains(t, out, "Status") | |||
| assert.Contains(t, out, "success") | |||
| } | |||
Comment on lines
1442
to
1458
| func TestClusterHealthCmd_TableOutputIgnoresQuiet(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| json.NewEncoder(w).Encode(types.ErrorResponse{Error: "Internal server error"}) | ||
| w.WriteHeader(http.StatusOK) | ||
| json.NewEncoder(w).Encode(sampleClusterHealth()) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| _ = setupClusterTestCmd(t, server.URL) | ||
| buf := setupClusterTestCmd(t, server.URL) | ||
| printer.Quiet = true | ||
|
|
||
| err := clusterHealthCmd.RunE(clusterHealthCmd, []string{"1"}) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "Internal server error") | ||
| require.NoError(t, err) | ||
| out := buf.String() | ||
| assert.Contains(t, out, "Health Status") | ||
| assert.Contains(t, out, "healthy") | ||
| } |
…alth - Restore quiet branch in test-connection: prints result.Status - Restore quiet branch in health: prints deriveHealthStatus(health) - Restore QuietOutput unit tests with exact string assertions - Restore UnknownWhenZeroNodes quiet assertion (unknown\n) - Fix ClusterTestConnectionResult docstring: 502 is surfaced as APIError, not as a populated struct field (Status == error never reaches caller)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
cli/test/integration/cluster_integration_test.go:103
- In the mock server,
len(parts)==3leavesactionempty unless the path is the special.../health/<sub>case. For any other 3-segment path under/api/v1/clusters/<id>/..., the handler falls through tocase action == "" && r.Method == GETand will incorrectly serve the cluster GET response instead of 404. To prevent tests from masking incorrect client URLs, consider explicitly 404'ing non-healthlen(parts)==3paths (or fully parsingaction/subaction).
switch len(parts) {
case 1:
id = parts[0]
case 2:
id = parts[0]
action = parts[1]
case 3:
// /api/v1/clusters/<id>/health/<sub> — handled below as a special case.
id = parts[0]
default:
w.WriteHeader(http.StatusNotFound)
json.NewEncoder(w).Encode(types.ErrorResponse{Error: "not found"})
return
}
Comment on lines
786
to
806
| var clusterNodesCmd = &cobra.Command{ | ||
| Use: "nodes <id>", | ||
| Short: "List nodes in a cluster", | ||
| Long: `List all nodes in a cluster with status, capacity, and pod count. | ||
|
|
||
| Examples: | ||
| stackctl cluster nodes 1 | ||
| stackctl cluster nodes 1 -o json`, | ||
| Use: "nodes <id>", | ||
| Short: "List nodes in a cluster with their status", | ||
| Long: "List per-node health, pod count, and allocatable CPU/memory for the cluster.", | ||
| Args: cobra.ExactArgs(1), | ||
| SilenceUsage: true, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| id, err := parseID(args[0]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| c, err := newClient() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| nodes, err := c.GetClusterNodes(id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if printer.Quiet { | ||
| for _, n := range nodes { | ||
| fmt.Fprintln(printer.Writer, n.Name) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| switch printer.Format { |
Comment on lines
828
to
848
| var clusterNamespacesCmd = &cobra.Command{ | ||
| Use: "namespaces <id>", | ||
| Short: "List stack namespaces in a cluster", | ||
| Long: `List all stack-* namespaces in a cluster. | ||
|
|
||
| Examples: | ||
| stackctl cluster namespaces 1 | ||
| stackctl cluster namespaces 1 -o json`, | ||
| Use: "namespaces <id>", | ||
| Short: "List stack-* namespaces present in a cluster", | ||
| Long: "List namespaces whose names start with stack-, along with their phase and creation time.", | ||
| Args: cobra.ExactArgs(1), | ||
| SilenceUsage: true, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| id, err := parseID(args[0]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| c, err := newClient() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| namespaces, err := c.GetClusterNamespaces(id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if printer.Quiet { | ||
| for _, ns := range namespaces { | ||
| fmt.Fprintln(printer.Writer, ns.Name) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| switch printer.Format { |
Comment on lines
868
to
889
| var clusterUtilizationCmd = &cobra.Command{ | ||
| Use: "utilization <id>", | ||
| Short: "Show per-namespace resource utilization for a cluster", | ||
| Long: `Show CPU, memory, and pod usage per namespace for a cluster. | ||
|
|
||
| Examples: | ||
| stackctl cluster utilization 1 | ||
| stackctl cluster utilization 1 -o json`, | ||
| Use: "utilization <id>", | ||
| Short: "Show per-namespace resource utilization", | ||
| Long: "Show aggregated CPU/memory/pod usage per stack-* namespace in the cluster.", | ||
| Args: cobra.ExactArgs(1), | ||
| SilenceUsage: true, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| id, err := parseID(args[0]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| c, err := newClient() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| util, err := c.GetClusterUtilization(id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, id) | ||
| return nil | ||
| } | ||
|
|
||
| switch printer.Format { | ||
| case output.FormatJSON: |
|
|
||
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, resp.Status) | ||
| fmt.Fprintln(printer.Writer, result.Status) |
|
|
||
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, id) | ||
| fmt.Fprintln(printer.Writer, deriveHealthStatus(health)) |
Comment on lines
1500
to
1517
| func TestClusterNodesCmd_TableOutputIgnoresQuiet(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) | ||
| json.NewEncoder(w).Encode([]types.ClusterNode{node}) | ||
| json.NewEncoder(w).Encode(sampleClusterNodes()) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| buf := setupClusterTestCmd(t, server.URL) | ||
| printer.Format = output.FormatJSON | ||
| printer.Quiet = true | ||
|
|
||
| err := clusterNodesCmd.RunE(clusterNodesCmd, []string{"1"}) | ||
| require.NoError(t, err) | ||
|
|
||
| var result []types.ClusterNode | ||
| require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) | ||
| require.Len(t, result, 1) | ||
| assert.Equal(t, "node-1", result[0].Name) | ||
| assert.Equal(t, "ready", result[0].Status) | ||
| out := buf.String() | ||
| assert.Contains(t, out, "node-a") | ||
| assert.Contains(t, out, "node-b") | ||
| assert.Contains(t, out, "NAME") | ||
| } |
Comment on lines
1590
to
1607
| func TestClusterNamespacesCmd_TableOutputIgnoresQuiet(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) | ||
| json.NewEncoder(w).Encode([]types.ClusterNamespace{ns}) | ||
| json.NewEncoder(w).Encode(sampleClusterNamespaces()) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| buf := setupClusterTestCmd(t, server.URL) | ||
| printer.Format = output.FormatJSON | ||
| printer.Quiet = true | ||
|
|
||
| err := clusterNamespacesCmd.RunE(clusterNamespacesCmd, []string{"1"}) | ||
| require.NoError(t, err) | ||
|
|
||
| var result []types.ClusterNamespace | ||
| require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) | ||
| require.Len(t, result, 1) | ||
| assert.Equal(t, "stack-dev-alice", result[0].Name) | ||
| assert.Equal(t, "Active", result[0].Phase) | ||
| out := buf.String() | ||
| assert.Contains(t, out, "stack-prod-web") | ||
| assert.Contains(t, out, "stack-dev-api") | ||
| assert.Contains(t, out, "NAME") | ||
| } |
Comment on lines
1684
to
1701
| func TestClusterUtilizationCmd_TableOutputIgnoresQuiet(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) | ||
| json.NewEncoder(w).Encode(util) | ||
| json.NewEncoder(w).Encode(sampleClusterUtilization()) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| buf := setupClusterTestCmd(t, server.URL) | ||
| printer.Format = output.FormatJSON | ||
| printer.Quiet = true | ||
|
|
||
| err := clusterUtilizationCmd.RunE(clusterUtilizationCmd, []string{"1"}) | ||
| require.NoError(t, err) | ||
|
|
||
| var result types.ClusterUtilization | ||
| require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) | ||
| assert.Equal(t, "1", result.ClusterID) | ||
| require.Len(t, result.Namespaces, 1) | ||
| assert.Equal(t, "stack-dev-alice", result.Namespaces[0].Namespace) | ||
| out := buf.String() | ||
| assert.Contains(t, out, "stack-prod") | ||
| assert.Contains(t, out, "stack-dev") | ||
| assert.Contains(t, out, "NAMESPACE") | ||
| } |
…tilization - Add quiet block to cluster nodes: prints node names via printer.PrintIDs - Add quiet block to cluster namespaces: prints namespace names via printer.PrintIDs - Add quiet block to cluster utilization: prints namespace names via printer.PrintIDs - Rename *TableOutputIgnoresQuiet tests to *QuietOutput with exact assertions (names only, assert.NotContains for table headers) - Update commands.instructions.md to document all quiet-mode exceptions for cluster subcommands so reviewers apply the contract consistently - Add .coderabbit.yaml to point CodeRabbit at instruction files per path
📝 WalkthroughWalkthroughAdds endpoint-specific cluster DTOs and updates client methods; changes cluster CLI outputs and quiet-mode exceptions; extends mock servers and adds unit/integration/E2E tests; updates documentation and adds CodeRabbit and auxiliary YAML/JSON configs. ChangesCluster Health and Status CLI Commands
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cli/pkg/client/client.go (1)
1053-1056: ⚡ Quick winAdd
@seeroute annotations to the new public client method commentsThese updated public method comments should include explicit
@see <HTTP_VERB> <route>entries for consistency with client conventions.As per coding guidelines: “Every public method has a TSDoc-style Go doc comment with
@seeHTTP verb + route”.Also applies to: 1065-1066, 1074-1075, 1083-1084
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/pkg/client/client.go` around lines 1053 - 1056, The public client docs for TestClusterConnection (and the three adjacent new public methods) are missing the required TSDoc-style `@see` annotations; update each public method comment (e.g., the TestClusterConnection comment and the comments immediately at the next three method declarations) to include an explicit "`@see` <HTTP_VERB> <route>" line that matches the HTTP endpoint the method calls (use the exact verb and path used by the client implementation for each method).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.coderabbit.yaml:
- Around line 7-8: The comment for the YAML key request_changes_workflow is
misleading; update the comment to state that this flag controls whether an
auto-approval workflow runs after requested changes or failing checks are
resolved (not review frequency on pushes). Locate the existing comment above
request_changes_workflow and replace it with a concise description like:
"Control auto-approval behavior after requested changes/checks are resolved
(true enables auto-approval workflow)." Keep the YAML key name
request_changes_workflow unchanged.
In @.github/instructions/commands.instructions.md:
- Line 41: Update the docs for the cluster test-connection command to avoid
implying quiet mode will print "error" on API failures; instead state that in
quiet mode the command writes the connection status string (via
fmt.Fprintln(printer.Writer, result.Status)) only for successful (2xx)
responses, while non-2xx responses are returned as command errors (and therefore
will not print "error" to stdout).
In `@cli/pkg/types/types.go`:
- Line 344: ClusterNamespace's CreatedAt and UpdatedAt are non-pointer time.Time
fields with `omitempty`, which doesn't omit zero times; change their types to
pointers (e.g., *time.Time) to match the file's pattern (like
DeletedAt/ExpiresAt) and keep `omitempty` OR, if you prefer Go 1.24+, keep them
as time.Time but change the struct tags to use `omitzero`
(json:"created_at,omitempty,omitzero" / json:"updated_at,omitempty,omitzero") so
zero timestamps are omitted; update both field types/tags for
ClusterNamespace.CreatedAt and ClusterNamespace.UpdatedAt consistently and
adjust any code that constructs or reads these fields to handle pointers if you
choose the pointer approach.
In `@cli/test/integration/cluster_integration_test.go`:
- Around line 96-99: The test HTTP handler currently treats any 3-segment path
as an id lookup (setting id = parts[0]) allowing requests like
/api/v1/clusters/<id>/foo/bar to fall through; update the handler to explicitly
reject 3-segment cluster paths whose second segment is not "health" before the
generic GET-by-id branch: when len(parts)==3 check parts[1] and return a 404 (or
the test-expected error) for non-"health" values, using the existing parts and
id variables and adjusting the same logic locations referenced around the switch
handling (the case handling len(parts)==3 and the generic GET-by-id branch).
---
Nitpick comments:
In `@cli/pkg/client/client.go`:
- Around line 1053-1056: The public client docs for TestClusterConnection (and
the three adjacent new public methods) are missing the required TSDoc-style `@see`
annotations; update each public method comment (e.g., the TestClusterConnection
comment and the comments immediately at the next three method declarations) to
include an explicit "`@see` <HTTP_VERB> <route>" line that matches the HTTP
endpoint the method calls (use the exact verb and path used by the client
implementation for each method).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf3a2ace-8679-4d41-b319-dbf81f865639
📒 Files selected for processing (9)
.coderabbit.yaml.github/instructions/commands.instructions.mdcli/cmd/cluster.gocli/cmd/cluster_test.gocli/pkg/client/client.gocli/pkg/client/client_test.gocli/pkg/types/types.gocli/test/e2e/cli_e2e_test.gocli/test/integration/cluster_integration_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/instructions/tests.instructions.md:
- Line 69: Update the live-test command path string in the documentation:
replace the incorrect `./test/live/` with the correct `cli/test/live/` so the
example command reads `go test -tags live ./cli/test/live/ -v`; ensure any other
occurrences in the same section referencing `./test/live/` are updated to
`cli/test/live/` for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08488102-a8cc-4986-bc88-957db0cc5af3
📒 Files selected for processing (4)
.coderabbit.yaml.github/instructions/output.instructions.md.github/instructions/tests.instructions.md.github/instructions/types.instructions.md
✅ Files skipped from review due to trivial changes (2)
- .github/instructions/output.instructions.md
- .github/instructions/types.instructions.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .coderabbit.yaml
…ath rejection, docs clarification
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/pkg/types/types.go`:
- Around line 288-299: Add contract-level godoc for each exported DTO listed
(ClusterHealthSummary, ClusterResourceQuantity, ClusterNodeCondition,
ClusterNodeStatus, ClusterNamespace, NamespaceResourceUsage, ClusterUtilization)
that states the exact API endpoint(s) that produce the type, whether fields are
always present or only populated in non-zero cases (call out each omitempty
field behavior), and note any types only returned for specific HTTP status
codes; update the top-of-type comments in types.go immediately above the
corresponding type declarations (e.g., ClusterHealthSummary,
ClusterResourceQuantity, etc.) to include the endpoint mapping and explicit
population rules so callers know when a field may be zero-valued versus
populated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 767b22c5-d7aa-49e0-9d05-f44164c74f60
📒 Files selected for processing (10)
.coderabbit.yaml.github/instructions/commands.instructions.md.github/instructions/tests.instructions.mdcli/cmd/cluster.gocli/cmd/cluster_test.gocli/pkg/client/client.gocli/pkg/types/types.gocli/test/integration/cluster_integration_test.goentities.jsonmempalace.yaml
✅ Files skipped from review due to trivial changes (2)
- entities.json
- mempalace.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/instructions/commands.instructions.md
- .coderabbit.yaml
- .github/instructions/tests.instructions.md
- cli/pkg/client/client.go
- cli/cmd/cluster.go
- cli/test/integration/cluster_integration_test.go
- cli/cmd/cluster_test.go
Addresses outstanding review threads on PR #83: - `Client.do()` previously decoded non-2xx bodies as `types.ErrorResponse{Error: ...}` only. The POST /api/v1/clusters/:id/test endpoint returns `{"status":"error","message":"..."}` on a 502 (unreachable cluster), so `apiErr.Message` was empty and users saw a generic "Server error. Check backend logs." with no context. Decoder now tries the `error` field first and falls back to `message`, surfacing the backend-provided context in `APIError.Message` and through the user-facing 5xx rendering. - The `ClusterTestConnectionResult` docstring previously claimed the 502 path returned this struct with `Status: "error"`. That was never decoded into the struct — the error path goes through `APIError`. Docstring rewritten to describe the actual decoder behavior. - Added contract-level godoc to every new exported DTO (`ClusterHealthSummary`, `ClusterTestConnectionResult`, `ClusterResourceQuantity`, `ClusterNodeCondition`, `ClusterNodeStatus`, `ClusterNamespace`, `NamespaceResourceUsage`, `ClusterUtilization`) per the `.github/instructions/types.instructions.md` contract: the producing endpoint, when fields are populated vs. zero-valued, and any HTTP-status-specific population rules. - `TestTestClusterConnection_Unreachable` now mocks the real `{"status":"error","message":"..."}` wire shape and asserts both `apiErr.Message` and the user-facing rendering carry the backend message — guarding the regression and validating the new decoder fallback path end-to-end through the client. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/pkg/client/client_test.go (2)
2100-2119: ⚡ Quick winAssert the remaining rewritten health fields here.
This contract test still leaves
TotalCPUandAllocatableMemoryunchecked, so those renamed wire fields can regress back to zero-values without tripping this suite.Suggested assertions
assert.Equal(t, 3, health.NodeCount) assert.Equal(t, 3, health.ReadyNodeCount) + assert.Equal(t, "8", health.TotalCPU) assert.Equal(t, "7", health.AllocatableCPU) assert.Equal(t, "16Gi", health.TotalMemory) + assert.Equal(t, "14Gi", health.AllocatableMemory) assert.Equal(t, 12, health.NamespaceCount)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/pkg/client/client_test.go` around lines 2100 - 2119, Add assertions in the test after calling c.GetClusterHealth("1") to verify the renamed/mapped fields TotalCPU and AllocatableMemory are populated correctly: check that health.TotalCPU equals "8" and health.AllocatableMemory equals "14Gi" (matching the types.ClusterHealthSummary values encoded by the test server). Update the test around New(server.URL) / c.GetClusterHealth("1") to include these two assertions so regressions of TotalCPU or AllocatableMemory are caught.
2229-2272: ⚡ Quick winAdd matching error-path tests for namespaces and utilization.
This section only exercises the happy path for
GetClusterNamespacesandGetClusterUtilization, so a regression in non-2xx propagation for either endpoint would now slip through unnoticed. As per coding guidelines, "Test error cases: API errors (401, 404, 500) and invalid input in unit tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/pkg/client/client_test.go` around lines 2229 - 2272, Add tests that exercise error paths for GetClusterNamespaces and GetClusterUtilization: extend TestGetClusterNamespaces_Success and TestGetClusterUtilization_Success (or add new tests) to simulate non-2xx server responses (e.g., 401/404/500) from the httptest server and assert that c.GetClusterNamespaces and c.GetClusterUtilization return a non-nil error and do not return successful data; also add tests for invalid input (e.g., empty cluster ID passed to New(...).GetClusterNamespaces and GetClusterUtilization) and assert proper error handling. Use the existing httptest.NewServer handler pattern and the client constructor New(server.URL) to locate where to inject the failing responses and checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/pkg/client/client_test.go`:
- Around line 2100-2119: Add assertions in the test after calling
c.GetClusterHealth("1") to verify the renamed/mapped fields TotalCPU and
AllocatableMemory are populated correctly: check that health.TotalCPU equals "8"
and health.AllocatableMemory equals "14Gi" (matching the
types.ClusterHealthSummary values encoded by the test server). Update the test
around New(server.URL) / c.GetClusterHealth("1") to include these two assertions
so regressions of TotalCPU or AllocatableMemory are caught.
- Around line 2229-2272: Add tests that exercise error paths for
GetClusterNamespaces and GetClusterUtilization: extend
TestGetClusterNamespaces_Success and TestGetClusterUtilization_Success (or add
new tests) to simulate non-2xx server responses (e.g., 401/404/500) from the
httptest server and assert that c.GetClusterNamespaces and
c.GetClusterUtilization return a non-nil error and do not return successful
data; also add tests for invalid input (e.g., empty cluster ID passed to
New(...).GetClusterNamespaces and GetClusterUtilization) and assert proper error
handling. Use the existing httptest.NewServer handler pattern and the client
constructor New(server.URL) to locate where to inject the failing responses and
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fd82515f-7132-48fa-9f3c-863861916d94
📒 Files selected for processing (3)
cli/pkg/client/client.gocli/pkg/client/client_test.gocli/pkg/types/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/pkg/types/types.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds five cluster subcommands wrapping already-shipped k8s-stack-manager endpoints:
stackctl cluster test-connection <id>POST /api/v1/clusters/:id/teststackctl cluster health <id>GET /api/v1/clusters/:id/health/summarystackctl cluster nodes <id>GET /api/v1/clusters/:id/health/nodesstackctl cluster namespaces <id>GET /api/v1/clusters/:id/namespacesstackctl cluster utilization <id>GET /api/v1/clusters/:id/utilizationEach command supports the standard
--output table|json|yamlplus--quiet. AderiveHealthStatushelper collapses the summary into one label —healthy(all nodes ready),degraded(some not ready),unknown(zero nodes registered).cli/pkg/types.ClusterHealthSummaryis rewritten. The previous fields (Status,CPUUsage,MemUsage,CPUTotal,MemTotal) didn't match the backend'sk8s.ClusterSummaryJSON shape, sostackctl cluster getwas silently rendering zero values for those fields. New shape mirrors the wire format:Existing CLI users see more health data in
cluster getafter upgrade, not less. Out-of-tree consumers ofcli/pkg/typesneed to update field references.Closes #65
Test plan
All three layers green locally;
go vet ./...clean.cluster_integration_test.gomock to handle the five new routes;TestClusterWorkflow_HealthSurfacewalks create → test → health → nodes → namespaces → utilization;TestClusterWorkflow_TestConnectionUnreachablecovers the 502 path;TestClusterCobra_Healthdrives Cobra in-process.TestE2EClusterHealthbuilds the binary and runsstackctl cluster health 1in table, json, and quiet modes against the httptest mock.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
unknown/healthy/degraded). Default test-connection table includes Message and Server Version.Bug Fixes
Documentation
Refactor
Tests
Chores