Skip to content

Commit 1f3b689

Browse files
ChrisJBurnsChrisJBurns
authored
moves GetToolConfigForMCPServer into separate package for easier testing and reducing code in controllers layer (#2344)
* moves tool into separate package for easier tests Signed-off-by: ChrisJBurns <[email protected]> * adds accidental deletion of tests Signed-off-by: ChrisJBurns <[email protected]> * removes unused code Signed-off-by: ChrisJBurns <[email protected]> * lint Signed-off-by: ChrisJBurns <[email protected]> --------- Signed-off-by: ChrisJBurns <[email protected]>
1 parent 2642816 commit 1f3b689

File tree

7 files changed

+255
-227
lines changed

7 files changed

+255
-227
lines changed

‎cmd/thv-operator/controllers/mcpserver_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -792,7 +792,7 @@ func (r *MCPServerReconciler) handleToolConfig(ctx context.Context, m *mcpv1alph
792792
}
793793

794794
// Get the referenced MCPToolConfig
795-
toolConfig, err := GetToolConfigForMCPServer(ctx, r.Client, m)
795+
toolConfig, err := ctrlutil.GetToolConfigForMCPServer(ctx, r.Client, m)
796796
if err != nil {
797797
return err
798798
}

‎cmd/thv-operator/controllers/mcpserver_runconfig.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ func (r *MCPServerReconciler) createRunConfigFromMCPServer(m *mcpv1alpha1.MCPSer
9494

9595
if m.Spec.ToolConfigRef != nil {
9696
// ToolConfigRef takes precedence over inline ToolsFilter
97-
toolConfig, err := GetToolConfigForMCPServer(context.Background(), r.Client, m)
97+
toolConfig, err := ctrlutil.GetToolConfigForMCPServer(context.Background(), r.Client, m)
9898
if err != nil {
9999
return nil, fmt.Errorf("failed to get MCPToolConfig: %w", err)
100100
}

‎cmd/thv-operator/controllers/toolconfig_controller.go‎

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -227,32 +227,3 @@ func (r *ToolConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
227227
Watches(&mcpv1alpha1.MCPServer{}, toolConfigHandler).
228228
Complete(r)
229229
}
230-
231-
// GetToolConfigForMCPServer retrieves the MCPToolConfig referenced by an MCPServer
232-
func GetToolConfigForMCPServer(
233-
ctx context.Context,
234-
c client.Client,
235-
mcpServer *mcpv1alpha1.MCPServer,
236-
) (*mcpv1alpha1.MCPToolConfig, error) {
237-
if mcpServer.Spec.ToolConfigRef == nil {
238-
// We throw an error because in this case you assume there is a ToolConfig
239-
// but there isn't one referenced.
240-
return nil, fmt.Errorf("MCPServer %s does not reference a MCPToolConfig", mcpServer.Name)
241-
}
242-
243-
toolConfig := &mcpv1alpha1.MCPToolConfig{}
244-
err := c.Get(ctx, types.NamespacedName{
245-
Name: mcpServer.Spec.ToolConfigRef.Name,
246-
Namespace: mcpServer.Namespace, // Same namespace as MCPServer
247-
}, toolConfig)
248-
249-
if err != nil {
250-
if errors.IsNotFound(err) {
251-
return nil, fmt.Errorf("MCPToolConfig %s not found in namespace %s",
252-
mcpServer.Spec.ToolConfigRef.Name, mcpServer.Namespace)
253-
}
254-
return nil, fmt.Errorf("failed to get MCPToolConfig: %w", err)
255-
}
256-
257-
return toolConfig, nil
258-
}

‎cmd/thv-operator/controllers/toolconfig_controller_edge_cases_test.go‎

Lines changed: 0 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,8 @@ import (
88

99
"github.com/stretchr/testify/assert"
1010
"github.com/stretchr/testify/require"
11-
apierrors "k8s.io/apimachinery/pkg/api/errors"
1211
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1312
"k8s.io/apimachinery/pkg/runtime"
14-
"k8s.io/apimachinery/pkg/runtime/schema"
1513
"k8s.io/apimachinery/pkg/types"
1614
"sigs.k8s.io/controller-runtime/pkg/client"
1715
"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -246,94 +244,6 @@ func (c *errorClient) List(ctx context.Context, list client.ObjectList, opts ...
246244
return c.Client.List(ctx, list, opts...)
247245
}
248246

249-
func TestGetToolConfigForMCPServer_ErrorScenarios(t *testing.T) {
250-
t.Parallel()
251-
252-
t.Run("toolconfig not found returns formatted error", func(t *testing.T) {
253-
t.Parallel()
254-
ctx := t.Context()
255-
256-
scheme := runtime.NewScheme()
257-
require.NoError(t, mcpv1alpha1.AddToScheme(scheme))
258-
259-
mcpServer := &mcpv1alpha1.MCPServer{
260-
ObjectMeta: metav1.ObjectMeta{
261-
Name: "test-server",
262-
Namespace: "default",
263-
},
264-
Spec: mcpv1alpha1.MCPServerSpec{
265-
Image: "test-image",
266-
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{
267-
Name: "missing-config",
268-
},
269-
},
270-
}
271-
272-
fakeClient := fake.NewClientBuilder().
273-
WithScheme(scheme).
274-
Build()
275-
276-
config, err := GetToolConfigForMCPServer(ctx, fakeClient, mcpServer)
277-
assert.Error(t, err)
278-
assert.Nil(t, config)
279-
assert.Contains(t, err.Error(), "MCPToolConfig missing-config not found")
280-
assert.Contains(t, err.Error(), "namespace default")
281-
})
282-
283-
t.Run("generic error is wrapped", func(t *testing.T) {
284-
t.Parallel()
285-
286-
ctx := t.Context()
287-
288-
scheme := runtime.NewScheme()
289-
require.NoError(t, mcpv1alpha1.AddToScheme(scheme))
290-
291-
mcpServer := &mcpv1alpha1.MCPServer{
292-
ObjectMeta: metav1.ObjectMeta{
293-
Name: "test-server",
294-
Namespace: "default",
295-
},
296-
Spec: mcpv1alpha1.MCPServerSpec{
297-
Image: "test-image",
298-
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{
299-
Name: "test-config",
300-
},
301-
},
302-
}
303-
304-
// Create a client that returns a generic error
305-
fakeClient := &errorGetClient{
306-
Client: fake.NewClientBuilder().
307-
WithScheme(scheme).
308-
Build(),
309-
getError: errors.New("network error"),
310-
}
311-
312-
config, err := GetToolConfigForMCPServer(ctx, fakeClient, mcpServer)
313-
assert.Error(t, err)
314-
assert.Nil(t, config)
315-
assert.Contains(t, err.Error(), "failed to get MCPToolConfig")
316-
assert.Contains(t, err.Error(), "network error")
317-
})
318-
}
319-
320-
// errorGetClient is a fake client that simulates Get errors
321-
type errorGetClient struct {
322-
client.Client
323-
getError error
324-
}
325-
326-
func (c *errorGetClient) Get(_ context.Context, key client.ObjectKey, _ client.Object, _ ...client.GetOption) error {
327-
if c.getError != nil {
328-
return c.getError
329-
}
330-
// Return not found error
331-
return apierrors.NewNotFound(schema.GroupResource{
332-
Group: "toolhive.stacklok.dev",
333-
Resource: "toolconfigs",
334-
}, key.Name)
335-
}
336-
337247
func TestToolConfigReconciler_ComplexScenarios(t *testing.T) {
338248
t.Parallel()
339249

‎cmd/thv-operator/controllers/toolconfig_controller_test.go‎

Lines changed: 0 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -304,109 +304,3 @@ func TestToolConfigReconciler_findReferencingMCPServers(t *testing.T) {
304304
assert.Contains(t, serverNames, "server2")
305305
assert.NotContains(t, serverNames, "server3")
306306
}
307-
308-
func TestGetToolConfigForMCPServer(t *testing.T) {
309-
t.Parallel()
310-
311-
tests := []struct {
312-
name string
313-
mcpServer *mcpv1alpha1.MCPServer
314-
existingConfig *mcpv1alpha1.MCPToolConfig
315-
expectConfig bool
316-
expectError bool
317-
}{
318-
{
319-
name: "mcpserver without toolconfig ref",
320-
mcpServer: &mcpv1alpha1.MCPServer{
321-
ObjectMeta: metav1.ObjectMeta{
322-
Name: "test-server",
323-
Namespace: "default",
324-
},
325-
Spec: mcpv1alpha1.MCPServerSpec{
326-
Image: "test-image",
327-
},
328-
},
329-
expectConfig: false,
330-
expectError: true, // Changed to expect an error when no ToolConfigRef is present
331-
},
332-
{
333-
name: "mcpserver with existing toolconfig",
334-
mcpServer: &mcpv1alpha1.MCPServer{
335-
ObjectMeta: metav1.ObjectMeta{
336-
Name: "test-server",
337-
Namespace: "default",
338-
},
339-
Spec: mcpv1alpha1.MCPServerSpec{
340-
Image: "test-image",
341-
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{
342-
Name: "test-config",
343-
},
344-
},
345-
},
346-
existingConfig: &mcpv1alpha1.MCPToolConfig{
347-
ObjectMeta: metav1.ObjectMeta{
348-
Name: "test-config",
349-
Namespace: "default",
350-
},
351-
Spec: mcpv1alpha1.MCPToolConfigSpec{
352-
ToolsFilter: []string{"tool1"},
353-
},
354-
},
355-
expectConfig: true,
356-
expectError: false,
357-
},
358-
{
359-
name: "mcpserver with non-existent toolconfig",
360-
mcpServer: &mcpv1alpha1.MCPServer{
361-
ObjectMeta: metav1.ObjectMeta{
362-
Name: "test-server",
363-
Namespace: "default",
364-
},
365-
Spec: mcpv1alpha1.MCPServerSpec{
366-
Image: "test-image",
367-
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{
368-
Name: "non-existent",
369-
},
370-
},
371-
},
372-
expectConfig: false,
373-
expectError: true,
374-
},
375-
}
376-
377-
for _, tt := range tests {
378-
t.Run(tt.name, func(t *testing.T) {
379-
t.Parallel()
380-
381-
ctx := t.Context()
382-
383-
scheme := runtime.NewScheme()
384-
require.NoError(t, mcpv1alpha1.AddToScheme(scheme))
385-
386-
objs := []client.Object{}
387-
if tt.existingConfig != nil {
388-
objs = append(objs, tt.existingConfig)
389-
}
390-
391-
fakeClient := fake.NewClientBuilder().
392-
WithScheme(scheme).
393-
WithObjects(objs...).
394-
Build()
395-
396-
config, err := GetToolConfigForMCPServer(ctx, fakeClient, tt.mcpServer)
397-
398-
if tt.expectError {
399-
assert.Error(t, err)
400-
assert.Nil(t, config)
401-
} else {
402-
assert.NoError(t, err)
403-
if tt.expectConfig {
404-
assert.NotNil(t, config)
405-
assert.Equal(t, tt.existingConfig.Name, config.Name)
406-
} else {
407-
assert.Nil(t, config)
408-
}
409-
}
410-
})
411-
}
412-
}

‎cmd/thv-operator/pkg/controllerutil/tools_config.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package controllerutil
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
"k8s.io/apimachinery/pkg/api/errors"
8+
"k8s.io/apimachinery/pkg/types"
9+
"sigs.k8s.io/controller-runtime/pkg/client"
10+
11+
mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
12+
)
13+
14+
// GetToolConfigForMCPServer retrieves the MCPToolConfig referenced by an MCPServer
15+
func GetToolConfigForMCPServer(
16+
ctx context.Context,
17+
c client.Client,
18+
mcpServer *mcpv1alpha1.MCPServer,
19+
) (*mcpv1alpha1.MCPToolConfig, error) {
20+
if mcpServer.Spec.ToolConfigRef == nil {
21+
// We throw an error because in this case you assume there is a ToolConfig
22+
// but there isn't one referenced.
23+
return nil, fmt.Errorf("MCPServer %s does not reference a MCPToolConfig", mcpServer.Name)
24+
}
25+
26+
toolConfig := &mcpv1alpha1.MCPToolConfig{}
27+
err := c.Get(ctx, types.NamespacedName{
28+
Name: mcpServer.Spec.ToolConfigRef.Name,
29+
Namespace: mcpServer.Namespace, // Same namespace as MCPServer
30+
}, toolConfig)
31+
32+
if err != nil {
33+
if errors.IsNotFound(err) {
34+
return nil, fmt.Errorf("MCPToolConfig %s not found in namespace %s",
35+
mcpServer.Spec.ToolConfigRef.Name, mcpServer.Namespace)
36+
}
37+
return nil, fmt.Errorf("failed to get MCPToolConfig: %w", err)
38+
}
39+
40+
return toolConfig, nil
41+
}

0 commit comments

Comments
 (0)