Ensure groups are removed from clients correctly (#1428)

Author: eleftherias

cmd/thv/app/client.go

@@ -17,7 +17,8 @@ import (
 )
 
 var (
-	groupNames []string
+	groupAddNames []string
+	groupRmNames  []string
 )
 
 var clientCmd = &cobra.Command{
@@ -104,9 +105,9 @@ func init() {
 
 	// TODO: Re-enable when group functionality is complete
 	//clientRegisterCmd.Flags().StringSliceVar(
-	//	&groupNames, "group", []string{"default"}, "Only register workloads from specified groups")
+	//	&groupAddNames, "group", []string{groups.DefaultGroup}, "Only register workloads from specified groups")
 	//clientRemoveCmd.Flags().StringSliceVar(
-	//	&groupNames, "group", []string{}, "Remove client from specified groups (if not set, removes all workloads from the client)")
+	//	&groupRmNames, "group", []string{}, "Remove client from specified groups (if not set, removes all workloads from the client)")
 }
 
 func clientStatusCmdFunc(_ *cobra.Command, _ []string) error {
@@ -193,7 +194,7 @@ func clientRegisterCmdFunc(cmd *cobra.Command, args []string) error {
 			clientType)
 	}
 
-	return performClientRegistration(cmd.Context(), []client.Client{{Name: client.MCPClient(clientType)}}, groupNames)
+	return performClientRegistration(cmd.Context(), []client.Client{{Name: client.MCPClient(clientType)}}, groupAddNames)
 }
 
 func clientRemoveCmdFunc(cmd *cobra.Command, args []string) error {
@@ -211,7 +212,7 @@ func clientRemoveCmdFunc(cmd *cobra.Command, args []string) error {
 			clientType)
 	}
 
-	return performClientRemoval(cmd.Context(), client.Client{Name: client.MCPClient(clientType)}, groupNames)
+	return performClientRemoval(cmd.Context(), client.Client{Name: client.MCPClient(clientType)}, groupRmNames)
 }
 
 func listRegisteredClientsCmdFunc(_ *cobra.Command, _ []string) error {
@@ -431,7 +432,6 @@ func removeClientGlobally(
 				return
 			}
 		}
-		logger.Warnf("Client %s was not found in registered clients list", clientToRemove.Name)
 	})
 	if err != nil {
 		return fmt.Errorf("failed to update configuration for client %s: %w", clientToRemove.Name, err)

pkg/api/v1/clients.go

@@ -437,7 +437,6 @@ func (c *ClientRoutes) removeClientGlobally(
 					return
 				}
 			}
-			logger.Warnf("Client %s was not found in registered clients list", clientToRemove.Name)
 		})
 		if err != nil {
 			return fmt.Errorf("failed to update configuration for client %s: %w", clientToRemove.Name, err)

pkg/client/manager.go

@@ -196,7 +196,7 @@ func (*defaultManager) removeServerFromClient(clientName MCPClient, serverName s
 		return fmt.Errorf("failed to remove MCP server configuration from %s: %v", clientConfig.Path, err)
 	}
 
-	logger.Infof("Removed MCP server %s from client %s\n", serverName, clientName)
+	logger.Infof("Removed MCP server %s from client %s", serverName, clientName)
 	return nil
 }
 
@@ -240,7 +240,7 @@ func (m *defaultManager) getTargetClients(ctx context.Context, serverName, group
 
 		logger.Infof(
 			"Server %s belongs to group %s, updating %d registered client(s)",
-			serverName, group, len(group.RegisteredClients),
+			serverName, group.Name, len(group.RegisteredClients),
 		)
 		return group.RegisteredClients
 	}

pkg/workloads/manager.go

@@ -418,21 +418,21 @@ func (d *defaultManager) cleanupWorkloadResources(childCtx context.Context, name
 		logger.Warnf("Warning: Failed to cleanup temporary permission profile: %v", err)
 	}
 
-	// Delete the saved state
+	// Remove client configurations
+	if err := removeClientConfigurations(name); err != nil {
+		logger.Warnf("Warning: Failed to remove client configurations: %v", err)
+	} else {
+		logger.Infof("Client configurations for %s removed", name)
+	}
+
+	// Delete the saved state last
 	if err := state.DeleteSavedRunConfig(childCtx, baseName); err != nil {
 		logger.Warnf("Warning: Failed to delete saved state: %v", err)
 	} else {
 		logger.Infof("Saved state for %s removed", baseName)
 	}
 
 	logger.Infof("Container %s removed", name)
-
-	// Remove client configurations
-	if err := removeClientConfigurations(name); err != nil {
-		logger.Warnf("Warning: Failed to remove client configurations: %v", err)
-	} else {
-		logger.Infof("Client configurations for %s removed", name)
-	}
 }
 
 func (d *defaultManager) DeleteWorkloads(ctx context.Context, names []string) (*errgroup.Group, error) {