Skip to content

Commit 6a9a3d2

Browse files
eleftheriaseleftherias
authored
Take groups into account when listing clients (#1430)
1 parent 0c548cd commit 6a9a3d2

File tree

9 files changed

+242
-33
lines changed

9 files changed

+242
-33
lines changed

‎cmd/thv/app/client.go

Lines changed: 47 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package app
33
import (
44
"context"
55
"fmt"
6-
"sort"
76

87
"github.com/spf13/cobra"
98

@@ -110,16 +109,16 @@ func init() {
110109
// &groupRmNames, "group", []string{}, "Remove client from specified groups (if not set, removes all workloads from the client)")
111110
}
112111

113-
func clientStatusCmdFunc(_ *cobra.Command, _ []string) error {
114-
clientStatuses, err := client.GetClientStatus()
112+
func clientStatusCmdFunc(cmd *cobra.Command, _ []string) error {
113+
clientStatuses, err := client.GetClientStatus(cmd.Context())
115114
if err != nil {
116115
return fmt.Errorf("failed to get client status: %w", err)
117116
}
118117
return ui.RenderClientStatusTable(clientStatuses)
119118
}
120119

121120
func clientSetupCmdFunc(cmd *cobra.Command, _ []string) error {
122-
clientStatuses, err := client.GetClientStatus()
121+
clientStatuses, err := client.GetClientStatus(cmd.Context())
123122
if err != nil {
124123
return fmt.Errorf("failed to get client status: %w", err)
125124
}
@@ -215,23 +214,55 @@ func clientRemoveCmdFunc(cmd *cobra.Command, args []string) error {
215214
return performClientRemoval(cmd.Context(), client.Client{Name: client.MCPClient(clientType)}, groupRmNames)
216215
}
217216

218-
func listRegisteredClientsCmdFunc(_ *cobra.Command, _ []string) error {
217+
func listRegisteredClientsCmdFunc(cmd *cobra.Command, _ []string) error {
219218
cfg := config.GetConfig()
220-
if len(cfg.Clients.RegisteredClients) == 0 {
221-
fmt.Println("No clients are currently registered.")
222-
return nil
219+
220+
// Check if we have groups configured
221+
groupManager, err := groups.NewManager()
222+
if err != nil {
223+
return fmt.Errorf("failed to create group manager: %w", err)
224+
}
225+
226+
allGroups, err := groupManager.List(cmd.Context())
227+
if err != nil {
228+
return fmt.Errorf("failed to list groups: %w", err)
229+
}
230+
231+
hasGroups := len(allGroups) > 0
232+
clientGroups := make(map[string][]string) // client -> groups
233+
allRegisteredClients := make(map[string]bool)
234+
235+
if hasGroups {
236+
// Collect clients from all groups
237+
for _, group := range allGroups {
238+
for _, clientName := range group.RegisteredClients {
239+
allRegisteredClients[clientName] = true
240+
clientGroups[clientName] = append(clientGroups[clientName], group.Name)
241+
}
242+
}
223243
}
224244

225-
// Create a copy of the registered clients and sort it alphabetically
226-
registeredClients := make([]string, len(cfg.Clients.RegisteredClients))
227-
copy(registeredClients, cfg.Clients.RegisteredClients)
228-
sort.Strings(registeredClients)
245+
// Add clients from global config that might not be in any group
246+
for _, clientName := range cfg.Clients.RegisteredClients {
247+
if !allRegisteredClients[clientName] {
248+
allRegisteredClients[clientName] = true
249+
if hasGroups {
250+
clientGroups[clientName] = []string{} // no groups
251+
}
252+
}
253+
}
229254

230-
fmt.Println("Registered clients:")
231-
for _, clientName := range registeredClients {
232-
fmt.Printf(" - %s\n", clientName)
255+
// Convert to slice for table rendering
256+
var registeredClients []ui.RegisteredClient
257+
for clientName := range allRegisteredClients {
258+
registered := ui.RegisteredClient{
259+
Name: clientName,
260+
Groups: clientGroups[clientName],
261+
}
262+
registeredClients = append(registeredClients, registered)
233263
}
234-
return nil
264+
265+
return ui.RenderRegisteredClientsTable(registeredClients, hasGroups)
235266
}
236267

237268
func performClientRegistration(ctx context.Context, clients []client.Client, groupNames []string) error {

‎cmd/thv/app/ui/clients_status.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package ui
33
import (
44
"fmt"
55
"os"
6+
"sort"
7+
"strings"
68

79
"github.com/olekukonko/tablewriter"
810
"github.com/olekukonko/tablewriter/tw"
@@ -56,3 +58,75 @@ func RenderClientStatusTable(clientStatuses []client.MCPClientStatus) error {
5658
}
5759
return nil
5860
}
61+
62+
// RegisteredClient represents a registered client with its associated groups
63+
type RegisteredClient struct {
64+
Name string
65+
Groups []string
66+
}
67+
68+
// RenderRegisteredClientsTable renders the registered clients table to stdout.
69+
func RenderRegisteredClientsTable(registeredClients []RegisteredClient, hasGroups bool) error {
70+
if len(registeredClients) == 0 {
71+
fmt.Println("No clients are currently registered.")
72+
return nil
73+
}
74+
75+
// Sort clients alphabetically by name
76+
sort.Slice(registeredClients, func(i, j int) bool {
77+
return registeredClients[i].Name < registeredClients[j].Name
78+
})
79+
80+
table := tablewriter.NewWriter(os.Stdout)
81+
82+
var headers []string
83+
if hasGroups {
84+
headers = []string{"Client Type", "Groups"}
85+
} else {
86+
headers = []string{"Client Type"}
87+
}
88+
89+
table.Options(
90+
tablewriter.WithHeader(headers),
91+
tablewriter.WithRendition(
92+
tw.Rendition{
93+
Borders: tw.Border{
94+
Left: tw.State(1),
95+
Top: tw.State(1),
96+
Right: tw.State(1),
97+
Bottom: tw.State(1),
98+
},
99+
},
100+
),
101+
tablewriter.WithAlignment(tw.MakeAlign(len(headers), tw.AlignLeft)),
102+
)
103+
104+
for _, regClient := range registeredClients {
105+
var row []string
106+
if hasGroups {
107+
groupsStr := ""
108+
if len(regClient.Groups) == 0 {
109+
// In practice, we should never get here
110+
groupsStr = "(no groups)"
111+
} else {
112+
// Sort groups alphabetically for consistency
113+
sortedGroups := make([]string, len(regClient.Groups))
114+
copy(sortedGroups, regClient.Groups)
115+
sort.Strings(sortedGroups)
116+
groupsStr = strings.Join(sortedGroups, ", ")
117+
}
118+
row = []string{regClient.Name, groupsStr}
119+
} else {
120+
row = []string{regClient.Name}
121+
}
122+
123+
if err := table.Append(row); err != nil {
124+
return fmt.Errorf("failed to append row: %w", err)
125+
}
126+
}
127+
128+
if err := table.Render(); err != nil {
129+
return fmt.Errorf("failed to render table: %w", err)
130+
}
131+
return nil
132+
}

‎pkg/api/v1/discovery.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ func DiscoveryRouter() http.Handler {
2929
// @Produce json
3030
// @Success 200 {object} clientStatusResponse
3131
// @Router /api/v1beta/discovery/clients [get]
32-
func (*DiscoveryRoutes) discoverClients(w http.ResponseWriter, _ *http.Request) {
33-
clients, err := client.GetClientStatus()
32+
func (*DiscoveryRoutes) discoverClients(w http.ResponseWriter, r *http.Request) {
33+
clients, err := client.GetClientStatus(r.Context())
3434
if err != nil {
3535
// TODO: Error should be JSON marshaled
3636
http.Error(w, "Failed to get client status", http.StatusInternalServerError)

‎pkg/client/config.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package client
44

55
import (
6+
"context"
67
"errors"
78
"fmt"
89
"os"
@@ -359,8 +360,8 @@ func FindClientConfig(clientType MCPClient) (*ConfigFile, error) {
359360
}
360361

361362
// FindRegisteredClientConfigs finds all registered client configs and creates them if they don't exist.
362-
func FindRegisteredClientConfigs() ([]ConfigFile, error) {
363-
clientStatuses, err := GetClientStatus()
363+
func FindRegisteredClientConfigs(ctx context.Context) ([]ConfigFile, error) {
364+
clientStatuses, err := GetClientStatus(ctx)
364365
if err != nil {
365366
return nil, fmt.Errorf("failed to get client status: %w", err)
366367
}

‎pkg/client/config_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package client
44

55
import (
66
"bytes"
7+
"context"
78
"io"
89
"os"
910
"path/filepath"
@@ -184,7 +185,7 @@ func TestFindClientConfigs(t *testing.T) { //nolint:paralleltest // Uses environ
184185

185186
// Find client configs - this should NOT fail due to the invalid JSON
186187
// Instead, it should log a warning and continue
187-
configs, err := FindRegisteredClientConfigs()
188+
configs, err := FindRegisteredClientConfigs(context.Background())
188189
assert.NoError(t, err, "FindRegisteredClientConfigs should not return an error for invalid config files")
189190

190191
// The invalid client should be skipped, so we should get configs for valid clients only
@@ -255,7 +256,7 @@ func TestSuccessfulClientConfigOperations(t *testing.T) {
255256
defer cleanup()
256257

257258
t.Run("FindAllConfiguredClients", func(t *testing.T) { //nolint:paralleltest // Uses environment variables
258-
configs, err := FindRegisteredClientConfigs()
259+
configs, err := FindRegisteredClientConfigs(context.Background())
259260
require.NoError(t, err)
260261
assert.Len(t, configs, len(supportedClientIntegrations), "Should find all mock client configs")
261262

@@ -272,7 +273,7 @@ func TestSuccessfulClientConfigOperations(t *testing.T) {
272273
})
273274

274275
t.Run("VerifyConfigFileContents", func(t *testing.T) { //nolint:paralleltest // Uses environment variables
275-
configs, err := FindRegisteredClientConfigs()
276+
configs, err := FindRegisteredClientConfigs(context.Background())
276277
require.NoError(t, err)
277278
require.NotEmpty(t, configs)
278279

@@ -326,7 +327,7 @@ func TestSuccessfulClientConfigOperations(t *testing.T) {
326327
})
327328

328329
t.Run("AddAndVerifyMCPServer", func(t *testing.T) { //nolint:paralleltest // Uses environment variables
329-
configs, err := FindRegisteredClientConfigs()
330+
configs, err := FindRegisteredClientConfigs(context.Background())
330331
require.NoError(t, err)
331332
require.NotEmpty(t, configs)
332333

‎pkg/client/discovery.go

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
package client
22

33
import (
4+
"context"
45
"fmt"
56
"os"
67
"path/filepath"
78
"runtime"
89
"sort"
910

1011
"github.com/stacklok/toolhive/pkg/config"
12+
"github.com/stacklok/toolhive/pkg/groups"
1113
)
1214

1315
// MCPClientStatus represents the status of a supported MCP client
@@ -23,7 +25,7 @@ type MCPClientStatus struct {
2325
}
2426

2527
// GetClientStatus returns the installation status of all supported MCP clients
26-
func GetClientStatus() ([]MCPClientStatus, error) {
28+
func GetClientStatus(ctx context.Context) ([]MCPClientStatus, error) {
2729
var statuses []MCPClientStatus
2830

2931
// Get home directory
@@ -36,11 +38,25 @@ func GetClientStatus() ([]MCPClientStatus, error) {
3638
appConfig := config.GetConfig()
3739
registeredClients := make(map[string]bool)
3840

39-
// Create a map of registered clients for quick lookup
41+
// Create a map of registered clients for quick lookup from global config
4042
for _, client := range appConfig.Clients.RegisteredClients {
4143
registeredClients[client] = true
4244
}
4345

46+
// Also check for clients registered in groups
47+
groupManager, err := groups.NewManager()
48+
if err == nil { // If group manager fails to initialize, we'll just skip group checks
49+
allGroups, err := groupManager.List(ctx)
50+
if err == nil {
51+
// Collect clients from all groups
52+
for _, group := range allGroups {
53+
for _, clientName := range group.RegisteredClients {
54+
registeredClients[clientName] = true
55+
}
56+
}
57+
}
58+
}
59+
4460
for _, cfg := range supportedClientIntegrations {
4561
status := MCPClientStatus{
4662
ClientType: cfg.ClientType,

0 commit comments

Comments
 (0)