diff --git a/cmd/docker-mcp/commands/workingset.go b/cmd/docker-mcp/commands/workingset.go index e3ad417de..e2ba96b15 100644 --- a/cmd/docker-mcp/commands/workingset.go +++ b/cmd/docker-mcp/commands/workingset.go @@ -128,14 +128,13 @@ To view enabled tools, use: docker mcp profile show `, func createWorkingSetCommand(cfg *client.Config) *cobra.Command { var opts struct { - ID string - Name string + Title string Servers []string Connect []string } cmd := &cobra.Command{ - Use: "create --name [--id ] --server --server ... [--connect --connect ...]", + Use: "create [--title ] [--server <ref1> --server <ref2> ...] [--connect <client1> --connect <client2> ...]", Short: "Create a new profile of MCP servers", Long: `Create a new profile that groups multiple MCP servers together. A profile allows you to organize and manage related servers as a single unit. @@ -145,34 +144,36 @@ Profiles are decoupled from catalogs. Servers can be: - Catalog references with catalog:// prefix (e.g., "catalog://mcp/docker-mcp-catalog/github+obsidian"). - Local file references with file:// prefix (e.g., "file://./server.yaml").`, Example: ` # Create a profile with servers from a catalog - docker mcp profile create --name dev-tools --server catalog://mcp/docker-mcp-catalog/github+obsidian + docker mcp profile create dev_tools --title "Dev Tools" --server catalog://mcp/docker-mcp-catalog/github+obsidian # Create a profile with multiple servers (OCI references) - docker mcp profile create --name my-profile --server docker://my-server:latest --server docker://my-other-server:latest + docker mcp profile create my_profile --title "My Profile" --server docker://my-server:latest --server docker://my-other-server:latest # Create a profile with MCP Registry references - docker mcp profile create --name my-profile --server http://registry.modelcontextprotocol.io/v0/servers/71de5a2a-6cfb-4250-a196-f93080ecc860 + docker mcp profile create my_profile --title "My Profile" --server http://registry.modelcontextprotocol.io/v0/servers/71de5a2a-6cfb-4250-a196-f93080ecc860 # Connect to clients upon creation - docker mcp profile create --name dev-tools --connect cursor`, - Args: cobra.NoArgs, - RunE: func(cmd *cobra.Command, _ []string) error { + docker mcp profile create dev_tools --title "Dev Tools" --connect cursor + + # Create profile without title (title will be same as id) + docker mcp profile create dev_tools --server docker://my-server:latest`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { dao, err := db.New() if err != nil { return err } registryClient := registryapi.NewClient() ociService := oci.NewService() - return workingset.Create(cmd.Context(), dao, registryClient, ociService, opts.ID, opts.Name, opts.Servers, opts.Connect) + profileID := args[0] + return workingset.Create(cmd.Context(), dao, registryClient, ociService, profileID, opts.Title, opts.Servers, opts.Connect) }, } flags := cmd.Flags() - flags.StringVar(&opts.Name, "name", "", "Name of the profile (required)") - flags.StringVar(&opts.ID, "id", "", "ID of the profile (defaults to a slugified version of the name)") + flags.StringVar(&opts.Title, "title", "", "Title of the profile (optional, defaults to profile-id if not provided)") flags.StringArrayVar(&opts.Servers, "server", []string{}, "Server to include specified with a URI: https:// (MCP Registry reference) or docker:// (Docker Image reference) or catalog:// (Catalog reference) or file:// (Local file path). Can be specified multiple times.") flags.StringArrayVar(&opts.Connect, "connect", []string{}, fmt.Sprintf("Clients to connect to: mcp-client (can be specified multiple times). Supported clients: %s", client.GetSupportedMCPClients(*cfg))) - _ = cmd.MarkFlagRequired("name") return cmd } @@ -416,31 +417,28 @@ func addServerCommand() *cobra.Command { } func removeServerCommand() *cobra.Command { - var names []string - cmd := &cobra.Command{ - Use: "remove <profile-id> --name <name1> --name <name2> ...", + Use: "remove <profile-id> <server-name>", Aliases: []string{"rm"}, - Short: "Remove MCP servers from a profile", - Long: "Remove MCP servers from a profile by server name.", - Example: ` # Remove servers by name - docker mcp profile server remove dev-tools --name github --name slack - - # Remove a single server - docker mcp profile server remove dev-tools --name github`, - Args: cobra.ExactArgs(1), + Short: "Remove MCP server from a profile", + Long: "Remove MCP server from a profile by server name.", + Example: ` # Remove a server by name + docker mcp profile server remove dev-tools github + + # Remove another server + docker mcp profile server remove dev-tools slack`, + Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { dao, err := db.New() if err != nil { return err } - return workingset.RemoveServers(cmd.Context(), dao, args[0], names) + profileID := args[0] + serverName := args[1] + return workingset.RemoveServers(cmd.Context(), dao, profileID, []string{serverName}) }, } - flags := cmd.Flags() - flags.StringArrayVar(&names, "name", []string{}, "Server name to remove (can be specified multiple times)") - return cmd } diff --git a/pkg/workingset/create.go b/pkg/workingset/create.go index a56ab174c..71807e8b7 100644 --- a/pkg/workingset/create.go +++ b/pkg/workingset/create.go @@ -31,20 +31,23 @@ func Create(ctx context.Context, dao db.DAO, registryClient registryapi.Client, } } - var err error - if id != "" { - _, err := dao.GetWorkingSet(ctx, id) - if err == nil { - return fmt.Errorf("profile with id %s already exists", id) - } - if !errors.Is(err, sql.ErrNoRows) { - return fmt.Errorf("failed to look for existing profile: %w", err) - } - } else { - id, err = createWorkingSetID(ctx, name, dao) - if err != nil { - return fmt.Errorf("failed to create profile id: %w", err) - } + // ID is mandatory + if id == "" { + return fmt.Errorf("id is required") + } + + // Check if profile with this ID already exists + _, err := dao.GetWorkingSet(ctx, id) + if err == nil { + return fmt.Errorf("profile with id %s already exists", id) + } + if !errors.Is(err, sql.ErrNoRows) { + return fmt.Errorf("failed to look for existing profile: %w", err) + } + + // Generate title from ID if not provided + if name == "" { + name = generateTitleFromID(id) } // Add default secrets diff --git a/pkg/workingset/create_test.go b/pkg/workingset/create_test.go index c6bd42b36..13f9391ab 100644 --- a/pkg/workingset/create_test.go +++ b/pkg/workingset/create_test.go @@ -94,7 +94,7 @@ func TestCreateWithDockerImages(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "My Test Set", []string{ + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "my_test_set", "My Test Set", []string{ "docker://myimage:latest", "docker://anotherimage:v1.0", }, []string{}) @@ -120,7 +120,7 @@ func TestCreateWithRegistryServers(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "Registry Set", []string{ + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "registry_set", "Registry Set", []string{ "https://example.com/v0/servers/server1", "https://example.com/v0/servers/server2", }, []string{}) @@ -144,7 +144,7 @@ func TestCreateWithMixedServers(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "Mixed Set", []string{ + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "mixed_set", "Mixed Set", []string{ "docker://myimage:latest", "https://example.com/v0/servers/server1", }, []string{}) @@ -196,32 +196,36 @@ func TestCreateWithExistingId(t *testing.T) { assert.Contains(t, err.Error(), "already exists") } -func TestCreateGeneratesUniqueIds(t *testing.T) { +func TestCreateGeneratesTitleFromId(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - // Create first working set - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "Test Set", []string{ + // Create working set with ID but no title + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "test_set_one", "", []string{ "docker://myimage:latest", }, []string{}) require.NoError(t, err) - // Create second with same name - err = Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "Test Set", []string{ - "docker://anotherimage:v1.0", - }, []string{}) + // Verify title equals ID when not explicitly provided + dbSet, err := dao.GetWorkingSet(ctx, "test_set_one") require.NoError(t, err) + assert.Equal(t, "test_set_one", dbSet.Name) - // Create third with same name - err = Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "Test Set", []string{ + // Create another with different ID + err = Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "my_profile_v2", "", []string{ "docker://anotherimage:v1.0", }, []string{}) require.NoError(t, err) + // Verify title equals ID when not explicitly provided + dbSet, err = dao.GetWorkingSet(ctx, "my_profile_v2") + require.NoError(t, err) + assert.Equal(t, "my_profile_v2", dbSet.Name) + // List all working sets sets, err := dao.ListWorkingSets(ctx) require.NoError(t, err) - assert.Len(t, sets, 3) + assert.Len(t, sets, 2) // Verify IDs are unique ids := make(map[string]bool) @@ -230,39 +234,43 @@ func TestCreateGeneratesUniqueIds(t *testing.T) { ids[set.ID] = true } - // Verify ID pattern - assert.Contains(t, ids, "test_set") - assert.Contains(t, ids, "test_set_2") - assert.Contains(t, ids, "test_set_3") + // Verify the expected IDs exist + assert.Contains(t, ids, "test_set_one") + assert.Contains(t, ids, "my_profile_v2") } func TestCreateWithInvalidServerFormat(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "Test Set", []string{ + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "test_set", "Test Set", []string{ "invalid-format", }, []string{}) require.Error(t, err) assert.Contains(t, err.Error(), "invalid server value") } -func TestCreateWithEmptyName(t *testing.T) { +func TestCreateWithEmptyTitle(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "test-id", "", []string{ + // Create profile with ID but no title - title should equal ID + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "test_id", "", []string{ "docker://myimage:latest", }, []string{}) - require.Error(t, err) - assert.Contains(t, err.Error(), "invalid profile") + require.NoError(t, err) + + // Verify title equals ID when not explicitly provided + dbSet, err := dao.GetWorkingSet(ctx, "test_id") + require.NoError(t, err) + assert.Equal(t, "test_id", dbSet.Name) } func TestCreateWithEmptyServers(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "Empty Set", []string{}, []string{}) + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "empty_set", "Empty Set", []string{}, []string{}) require.NoError(t, err) // Verify the working set was created with no servers @@ -277,7 +285,7 @@ func TestCreateAddsDefaultSecrets(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", "Test Set", []string{ + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "test_set", "Test Set", []string{ "docker://myimage:latest", }, []string{}) require.NoError(t, err) @@ -292,31 +300,31 @@ func TestCreateAddsDefaultSecrets(t *testing.T) { assert.Equal(t, "docker-desktop-store", dbSet.Secrets["default"].Provider) } -func TestCreateNameWithSpecialCharacters(t *testing.T) { +func TestCreateTitleGenerationFromID(t *testing.T) { tests := []struct { - name string - inputName string - expectedID string + name string + inputID string + expectedTitle string }{ { - name: "name with spaces", - inputName: "My Test Set", - expectedID: "my_test_set", + name: "id with underscores", + inputID: "my_test_set", + expectedTitle: "my_test_set", }, { - name: "name with special chars", - inputName: "Test@Set#123!", - expectedID: "test_set_123_", + name: "id with multiple underscores", + inputID: "test_set_123", + expectedTitle: "test_set_123", }, { - name: "name with multiple spaces", - inputName: "Test Set", - expectedID: "test_set", + name: "simple id", + inputID: "testset", + expectedTitle: "testset", }, { - name: "name with underscores", - inputName: "Test_Set_Name", - expectedID: "test_set_name", + name: "id with numbers", + inputID: "test_set_v2", + expectedTitle: "test_set_v2", }, } @@ -326,16 +334,17 @@ func TestCreateNameWithSpecialCharacters(t *testing.T) { dao := setupTestDB(t) ctx := t.Context() - err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), "", tt.inputName, []string{ + // Create profile with ID but no title - title should equal ID + err := Create(ctx, dao, getMockRegistryClient(), getMockOciService(), tt.inputID, "", []string{ "docker://myimage:latest", }, []string{}) require.NoError(t, err) - // Verify the ID was generated correctly - dbSet, err := dao.GetWorkingSet(ctx, tt.expectedID) + // Verify the title equals ID when not explicitly provided + dbSet, err := dao.GetWorkingSet(ctx, tt.inputID) require.NoError(t, err) require.NotNil(t, dbSet) - assert.Equal(t, tt.expectedID, dbSet.ID) + assert.Equal(t, tt.expectedTitle, dbSet.Name) }) } } diff --git a/pkg/workingset/list.go b/pkg/workingset/list.go index 8e4665caf..157f38698 100644 --- a/pkg/workingset/list.go +++ b/pkg/workingset/list.go @@ -71,7 +71,7 @@ func printListHumanReadable(workingSets []WorkingSet, showPolicy bool) string { } lines = strings.TrimSuffix(lines, "\n") if showPolicy { - return fmt.Sprintf("ID\tName\tPolicy\n----\t----\t------\n%s", lines) + return fmt.Sprintf("ID\tTitle\tPolicy\n----\t-----\t------\n%s", lines) } - return fmt.Sprintf("ID\tName\n----\t----\n%s", lines) + return fmt.Sprintf("ID\tTitle\n----\t-----\n%s", lines) } diff --git a/pkg/workingset/list_test.go b/pkg/workingset/list_test.go index a77e32ac9..608eae104 100644 --- a/pkg/workingset/list_test.go +++ b/pkg/workingset/list_test.go @@ -70,8 +70,8 @@ func TestListHumanReadable(t *testing.T) { }) // Verify header - assert.Contains(t, output, "ID\tName") - assert.Contains(t, output, "----\t----") + assert.Contains(t, output, "ID\tTitle") + assert.Contains(t, output, "----\t-----") // Verify data assert.Contains(t, output, "set-1\tFirst Set") diff --git a/pkg/workingset/server.go b/pkg/workingset/server.go index b214dfe5f..f13921c0d 100644 --- a/pkg/workingset/server.go +++ b/pkg/workingset/server.go @@ -56,7 +56,32 @@ func AddServers(ctx context.Context, dao db.DAO, registryClient registryapi.Clie RegisterOAuthProvidersForServers(ctx, newServers) - workingSet.Servers = append(workingSet.Servers, newServers...) + // Implement upsert behavior: if a server with the same name already exists, replace it + addedCount := 0 + updatedCount := 0 + for _, newServer := range newServers { + if newServer.Snapshot == nil { + return fmt.Errorf("server missing required snapshot metadata - this indicates a problem with server resolution") + } + + // Check if server with this name already exists + found := false + for i, existingServer := range workingSet.Servers { + if existingServer.Snapshot != nil && existingServer.Snapshot.Server.Name == newServer.Snapshot.Server.Name { + // Replace existing server + workingSet.Servers[i] = newServer + found = true + updatedCount++ + break + } + } + + // If not found, append + if !found { + workingSet.Servers = append(workingSet.Servers, newServer) + addedCount++ + } + } if err := workingSet.Validate(); err != nil { return fmt.Errorf("invalid profile: %w", err) @@ -67,7 +92,11 @@ func AddServers(ctx context.Context, dao db.DAO, registryClient registryapi.Clie return fmt.Errorf("failed to update profile: %w", err) } - fmt.Printf("Added %d server(s) to profile %s\n", len(newServers), id) + if updatedCount > 0 { + fmt.Printf("Added %d server(s) and updated %d server(s) in profile %s\n", addedCount, updatedCount, id) + } else { + fmt.Printf("Added %d server(s) to profile %s\n", addedCount, id) + } return nil } @@ -277,10 +306,10 @@ func printSearchResultsHuman(results []SearchResult, showPolicy bool) { } if showPolicy { - header := []string{"PROFILE", "TYPE", "IDENTIFIER", "POLICY"} + header := []string{"PROFILE", "TYPE", "NAME", "POLICY"} formatting.PrettyPrintTable(rows, []int{40, 10, 120, 10}, header) } else { - header := []string{"PROFILE", "TYPE", "IDENTIFIER"} + header := []string{"PROFILE", "TYPE", "NAME"} formatting.PrettyPrintTable(rows, []int{40, 10, 120}, header) } } diff --git a/pkg/workingset/workingset.go b/pkg/workingset/workingset.go index da140d471..f949836f5 100644 --- a/pkg/workingset/workingset.go +++ b/pkg/workingset/workingset.go @@ -348,6 +348,12 @@ func (s *Server) BasicName() string { return "unknown" } +// generateTitleFromID returns the profile ID unchanged +// The title is only different from the ID if the user explicitly provides --title +func generateTitleFromID(id string) string { + return id +} + func createWorkingSetID(ctx context.Context, name string, dao db.DAO) (string, error) { // Replace all non-alphanumeric characters with an underscore and make all uppercase lowercase re := regexp.MustCompile("[^a-zA-Z0-9]+")