Skip to content

Commit 95d49b8

Browse files
JAORMXclaude
JAORMX
and
claude
committed
Address PR review comments and fix CI issues
- Fix operator-crds README.md that was accidentally emptied - Add comprehensive unit tests for composite tool parameter conversion - Improve error handling documentation in converter.go - Bump toolhive-operator Helm chart version from 0.5.2 to 0.5.3 - Update all documentation to use standard JSON Schema format for parameters 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 77dd81e commit 95d49b8

File tree

8 files changed

+413
-67
lines changed

8 files changed

+413
-67
lines changed

‎cmd/thv-operator/pkg/vmcpconfig/converter.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,9 @@ func (*Converter) convertCompositeTools(
259259
if err := json.Unmarshal(crdTool.Parameters.Raw, &params); err == nil {
260260
tool.Parameters = params
261261
}
262-
// If unmarshal fails, leave Parameters as nil
262+
// If unmarshal fails, leave Parameters as nil. This should never happen in practice
263+
// because the webhook validation ensures Parameters contains valid JSON before reaching
264+
// this conversion point. See virtualmcpcompositetooldefinition_webhook.go for validation.
263265
}
264266

265267
// Convert steps

‎cmd/thv-operator/pkg/vmcpconfig/converter_test.go‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
package vmcpconfig
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"testing"
7+
8+
"github.com/stretchr/testify/assert"
9+
"github.com/stretchr/testify/require"
10+
"k8s.io/apimachinery/pkg/runtime"
11+
12+
mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
13+
)
14+
15+
func TestConverter_convertCompositeTools(t *testing.T) {
16+
t.Parallel()
17+
18+
tests := []struct {
19+
name string
20+
compositeTools []mcpv1alpha1.CompositeToolSpec
21+
wantLen int
22+
wantTool *struct {
23+
name string
24+
params map[string]any
25+
stepsCount int
26+
}
27+
}{
28+
{
29+
name: "valid parameters as JSON Schema",
30+
compositeTools: []mcpv1alpha1.CompositeToolSpec{
31+
{
32+
Name: "test-tool",
33+
Description: "A test tool",
34+
Parameters: &runtime.RawExtension{
35+
Raw: mustMarshalJSON(t, map[string]any{
36+
"type": "object",
37+
"properties": map[string]any{
38+
"param1": map[string]any{
39+
"type": "string",
40+
"default": "value1",
41+
},
42+
},
43+
"required": []string{"param1"},
44+
}),
45+
},
46+
Steps: []mcpv1alpha1.WorkflowStep{
47+
{
48+
ID: "step1",
49+
Type: "tool",
50+
Tool: "some-tool",
51+
},
52+
},
53+
},
54+
},
55+
wantLen: 1,
56+
wantTool: &struct {
57+
name string
58+
params map[string]any
59+
stepsCount int
60+
}{
61+
name: "test-tool",
62+
params: map[string]any{
63+
"type": "object",
64+
"properties": map[string]any{
65+
"param1": map[string]any{
66+
"type": "string",
67+
"default": "value1",
68+
},
69+
},
70+
"required": []any{"param1"},
71+
},
72+
stepsCount: 1,
73+
},
74+
},
75+
{
76+
name: "nil parameters",
77+
compositeTools: []mcpv1alpha1.CompositeToolSpec{
78+
{
79+
Name: "test-tool-no-params",
80+
Description: "A test tool without params",
81+
Parameters: nil,
82+
Steps: []mcpv1alpha1.WorkflowStep{
83+
{
84+
ID: "step1",
85+
Type: "tool",
86+
Tool: "some-tool",
87+
},
88+
},
89+
},
90+
},
91+
wantLen: 1,
92+
wantTool: &struct {
93+
name string
94+
params map[string]any
95+
stepsCount int
96+
}{
97+
name: "test-tool-no-params",
98+
params: nil,
99+
stepsCount: 1,
100+
},
101+
},
102+
{
103+
name: "empty parameters",
104+
compositeTools: []mcpv1alpha1.CompositeToolSpec{
105+
{
106+
Name: "test-tool-empty",
107+
Description: "A test tool with empty params",
108+
Parameters: &runtime.RawExtension{Raw: []byte("{}")},
109+
Steps: []mcpv1alpha1.WorkflowStep{
110+
{
111+
ID: "step1",
112+
Type: "tool",
113+
Tool: "some-tool",
114+
},
115+
},
116+
},
117+
},
118+
wantLen: 1,
119+
wantTool: &struct {
120+
name string
121+
params map[string]any
122+
stepsCount int
123+
}{
124+
name: "test-tool-empty",
125+
params: map[string]any{},
126+
stepsCount: 1,
127+
},
128+
},
129+
{
130+
name: "invalid JSON parameters - should be ignored (webhook validates before this)",
131+
compositeTools: []mcpv1alpha1.CompositeToolSpec{
132+
{
133+
Name: "test-tool-invalid",
134+
Description: "A test tool with invalid JSON",
135+
Parameters: &runtime.RawExtension{Raw: []byte("invalid json{")},
136+
Steps: []mcpv1alpha1.WorkflowStep{
137+
{
138+
ID: "step1",
139+
Type: "tool",
140+
Tool: "some-tool",
141+
},
142+
},
143+
},
144+
},
145+
wantLen: 1,
146+
wantTool: &struct {
147+
name string
148+
params map[string]any
149+
stepsCount int
150+
}{
151+
name: "test-tool-invalid",
152+
params: nil, // Invalid JSON results in nil params
153+
stepsCount: 1,
154+
},
155+
},
156+
}
157+
158+
for _, tt := range tests {
159+
t.Run(tt.name, func(t *testing.T) {
160+
t.Parallel()
161+
162+
converter := NewConverter()
163+
vmcp := &mcpv1alpha1.VirtualMCPServer{
164+
Spec: mcpv1alpha1.VirtualMCPServerSpec{
165+
CompositeTools: tt.compositeTools,
166+
},
167+
}
168+
169+
result := converter.convertCompositeTools(context.Background(), vmcp)
170+
171+
require.Len(t, result, tt.wantLen)
172+
if tt.wantTool != nil {
173+
assert.Equal(t, tt.wantTool.name, result[0].Name)
174+
assert.Equal(t, tt.wantTool.params, result[0].Parameters)
175+
assert.Len(t, result[0].Steps, tt.wantTool.stepsCount)
176+
}
177+
})
178+
}
179+
}
180+
181+
func mustMarshalJSON(t *testing.T, v any) []byte {
182+
t.Helper()
183+
data, err := json.Marshal(v)
184+
require.NoError(t, err)
185+
return data
186+
}

‎deploy/charts/operator-crds/README.md‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,43 @@
1-
\n
1+
2+
# ToolHive Operator CRDs Helm Chart
3+
4+
![Version: 0.0.60](https://img.shields.io/badge/Version-0.0.60-informational?style=flat-square)
5+
![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square)
6+
7+
A Helm chart for installing the ToolHive Operator CRDs into Kubernetes.
8+
9+
---
10+
11+
ToolHive Operator CRDs
12+
13+
## TL;DR
14+
15+
```console
16+
helm upgrade -i toolhive-operator-crds oci://ghcr.io/stacklok/toolhive/toolhive-operator-crds
17+
```
18+
19+
## Prerequisites
20+
21+
- Kubernetes 1.25+
22+
- Helm 3.10+ minimum, 3.14+ recommended
23+
24+
## Usage
25+
26+
### Installing from the Chart
27+
28+
Install one of the available versions:
29+
30+
```shell
31+
helm upgrade -i <release_name> oci://ghcr.io/stacklok/toolhive/toolhive-operator-crds --version=<version>
32+
```
33+
34+
> **Tip**: List all releases using `helm list`
35+
36+
### Uninstalling the Chart
37+
38+
To uninstall/delete the `toolhive-operator-crds` deployment:
39+
40+
```console
41+
helm uninstall <release_name>
42+
```
43+

‎deploy/charts/operator/Chart.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,5 @@ apiVersion: v2
22
name: toolhive-operator
33
description: A Helm chart for deploying the ToolHive Operator into Kubernetes.
44
type: application
5-
version: 0.5.2
5+
version: 0.5.3
66
appVersion: "v0.6.5"

‎deploy/charts/operator/README.md‎

Lines changed: 105 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,105 @@
1-
\n
1+
2+
# ToolHive Operator Helm Chart
3+
4+
![Version: 0.5.3](https://img.shields.io/badge/Version-0.5.3-informational?style=flat-square)
5+
![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square)
6+
7+
A Helm chart for deploying the ToolHive Operator into Kubernetes.
8+
9+
---
10+
11+
## TL;DR
12+
13+
```console
14+
helm upgrade -i toolhive-operator oci://ghcr.io/stacklok/toolhive/toolhive-operator -n toolhive-system --create-namespace
15+
```
16+
17+
Or for a custom values file:
18+
19+
```consoleCustom
20+
helm upgrade -i toolhive-operator oci://ghcr.io/stacklok/toolhive/toolhive-operator -n toolhive-system --create-namespace --values values-openshift.yaml
21+
```
22+
23+
## Prerequisites
24+
25+
- Kubernetes 1.25+
26+
- Helm 3.10+ minimum, 3.14+ recommended
27+
28+
## Usage
29+
30+
### Installing from the Chart
31+
32+
Install one of the available versions:
33+
34+
```shell
35+
helm upgrade -i <release_name> oci://ghcr.io/stacklok/toolhive/toolhive-operator --version=<version> -n toolhive-system --create-namespace
36+
```
37+
38+
> **Tip**: List all releases using `helm list`
39+
40+
### Uninstalling the Chart
41+
42+
To uninstall/delete the `toolhive-operator` deployment:
43+
44+
```console
45+
helm uninstall <release_name>
46+
```
47+
48+
The command removes all the Kubernetes components associated with the chart and deletes the release. You will have to delete the namespace manually if you used Helm to create it.
49+
50+
## Values
51+
52+
| Key | Type | Default | Description |
53+
|-----|-------------|------|---------|
54+
| fullnameOverride | string | `"toolhive-operator"` | Provide a fully-qualified name override for resources |
55+
| nameOverride | string | `""` | Override the name of the chart |
56+
| operator | object | `{"affinity":{},"autoscaling":{"enabled":false,"maxReplicas":100,"minReplicas":1,"targetCPUUtilizationPercentage":80},"containerSecurityContext":{"allowPrivilegeEscalation":false,"capabilities":{"drop":["ALL"]},"readOnlyRootFilesystem":true,"runAsNonRoot":true,"runAsUser":1000,"seccompProfile":{"type":"RuntimeDefault"}},"env":{},"features":{"experimental":false},"gc":{"gogc":75,"gomeglimit":"150MiB"},"image":"ghcr.io/stacklok/toolhive/operator:v0.6.5","imagePullPolicy":"IfNotPresent","imagePullSecrets":[],"leaderElectionRole":{"binding":{"name":"toolhive-operator-leader-election-rolebinding"},"name":"toolhive-operator-leader-election-role","rules":[{"apiGroups":[""],"resources":["configmaps"],"verbs":["get","list","watch","create","update","patch","delete"]},{"apiGroups":["coordination.k8s.io"],"resources":["leases"],"verbs":["get","list","watch","create","update","patch","delete"]},{"apiGroups":[""],"resources":["events"],"verbs":["create","patch"]}]},"livenessProbe":{"httpGet":{"path":"/healthz","port":"health"},"initialDelaySeconds":15,"periodSeconds":20},"nodeSelector":{},"podAnnotations":{},"podLabels":{},"podSecurityContext":{"runAsNonRoot":true},"ports":[{"containerPort":8080,"name":"metrics","protocol":"TCP"},{"containerPort":8081,"name":"health","protocol":"TCP"}],"proxyHost":"0.0.0.0","rbac":{"allowedNamespaces":[],"scope":"cluster"},"readinessProbe":{"httpGet":{"path":"/readyz","port":"health"},"initialDelaySeconds":5,"periodSeconds":10},"replicaCount":1,"resources":{"limits":{"cpu":"500m","memory":"128Mi"},"requests":{"cpu":"10m","memory":"64Mi"}},"serviceAccount":{"annotations":{},"automountServiceAccountToken":true,"create":true,"labels":{},"name":"toolhive-operator"},"tolerations":[],"toolhiveRunnerImage":"ghcr.io/stacklok/toolhive/proxyrunner:v0.6.5","volumeMounts":[],"volumes":[]}` | All values for the operator deployment and associated resources |
57+
| operator.affinity | object | `{}` | Affinity settings for the operator pod |
58+
| operator.autoscaling | object | `{"enabled":false,"maxReplicas":100,"minReplicas":1,"targetCPUUtilizationPercentage":80}` | Configuration for horizontal pod autoscaling |
59+
| operator.autoscaling.enabled | bool | `false` | Enable autoscaling for the operator |
60+
| operator.autoscaling.maxReplicas | int | `100` | Maximum number of replicas |
61+
| operator.autoscaling.minReplicas | int | `1` | Minimum number of replicas |
62+
| operator.autoscaling.targetCPUUtilizationPercentage | int | `80` | Target CPU utilization percentage for autoscaling |
63+
| operator.containerSecurityContext | object | `{"allowPrivilegeEscalation":false,"capabilities":{"drop":["ALL"]},"readOnlyRootFilesystem":true,"runAsNonRoot":true,"runAsUser":1000,"seccompProfile":{"type":"RuntimeDefault"}}` | Container security context settings for the operator |
64+
| operator.env | object | `{}` | Environment variables to set in the operator container |
65+
| operator.gc | object | `{"gogc":75,"gomeglimit":"150MiB"}` | Go memory limits and garbage collection percentage for the operator container |
66+
| operator.gc.gogc | int | `75` | Go garbage collection percentage for the operator container |
67+
| operator.gc.gomeglimit | string | `"150MiB"` | Go memory limits for the operator container |
68+
| operator.image | string | `"ghcr.io/stacklok/toolhive/operator:v0.6.5"` | Container image for the operator |
69+
| operator.imagePullPolicy | string | `"IfNotPresent"` | Image pull policy for the operator container |
70+
| operator.imagePullSecrets | list | `[]` | List of image pull secrets to use |
71+
| operator.leaderElectionRole | object | `{"binding":{"name":"toolhive-operator-leader-election-rolebinding"},"name":"toolhive-operator-leader-election-role","rules":[{"apiGroups":[""],"resources":["configmaps"],"verbs":["get","list","watch","create","update","patch","delete"]},{"apiGroups":["coordination.k8s.io"],"resources":["leases"],"verbs":["get","list","watch","create","update","patch","delete"]},{"apiGroups":[""],"resources":["events"],"verbs":["create","patch"]}]}` | Leader election role configuration |
72+
| operator.leaderElectionRole.binding.name | string | `"toolhive-operator-leader-election-rolebinding"` | Name of the role binding for leader election |
73+
| operator.leaderElectionRole.name | string | `"toolhive-operator-leader-election-role"` | Name of the role for leader election |
74+
| operator.leaderElectionRole.rules | list | `[{"apiGroups":[""],"resources":["configmaps"],"verbs":["get","list","watch","create","update","patch","delete"]},{"apiGroups":["coordination.k8s.io"],"resources":["leases"],"verbs":["get","list","watch","create","update","patch","delete"]},{"apiGroups":[""],"resources":["events"],"verbs":["create","patch"]}]` | Rules for the leader election role |
75+
| operator.livenessProbe | object | `{"httpGet":{"path":"/healthz","port":"health"},"initialDelaySeconds":15,"periodSeconds":20}` | Liveness probe configuration for the operator |
76+
| operator.nodeSelector | object | `{}` | Node selector for the operator pod |
77+
| operator.podAnnotations | object | `{}` | Annotations to add to the operator pod |
78+
| operator.podLabels | object | `{}` | Labels to add to the operator pod |
79+
| operator.podSecurityContext | object | `{"runAsNonRoot":true}` | Pod security context settings |
80+
| operator.ports | list | `[{"containerPort":8080,"name":"metrics","protocol":"TCP"},{"containerPort":8081,"name":"health","protocol":"TCP"}]` | List of ports to expose from the operator container |
81+
| operator.proxyHost | string | `"0.0.0.0"` | Host for the proxy deployed by the operator |
82+
| operator.rbac | object | `{"allowedNamespaces":[],"scope":"cluster"}` | RBAC configuration for the operator |
83+
| operator.rbac.allowedNamespaces | list | `[]` | List of namespaces that the operator is allowed to have permissions to manage. Only used if scope is set to "namespace". |
84+
| operator.rbac.scope | string | `"cluster"` | Scope of the RBAC configuration. - cluster: The operator will have cluster-wide permissions via ClusterRole and ClusterRoleBinding. - namespace: The operator will have permissions to manage resources in the namespaces specified in `allowedNamespaces`. The operator will have a ClusterRole and RoleBinding for each namespace in `allowedNamespaces`. |
85+
| operator.readinessProbe | object | `{"httpGet":{"path":"/readyz","port":"health"},"initialDelaySeconds":5,"periodSeconds":10}` | Readiness probe configuration for the operator |
86+
| operator.replicaCount | int | `1` | Number of replicas for the operator deployment |
87+
| operator.resources | object | `{"limits":{"cpu":"500m","memory":"128Mi"},"requests":{"cpu":"10m","memory":"64Mi"}}` | Resource requests and limits for the operator container |
88+
| operator.serviceAccount | object | `{"annotations":{},"automountServiceAccountToken":true,"create":true,"labels":{},"name":"toolhive-operator"}` | Service account configuration for the operator |
89+
| operator.serviceAccount.annotations | object | `{}` | Annotations to add to the service account |
90+
| operator.serviceAccount.automountServiceAccountToken | bool | `true` | Automatically mount a ServiceAccount's API credentials |
91+
| operator.serviceAccount.create | bool | `true` | Specifies whether a service account should be created |
92+
| operator.serviceAccount.labels | object | `{}` | Labels to add to the service account |
93+
| operator.serviceAccount.name | string | `"toolhive-operator"` | The name of the service account to use. If not set and create is true, a name is generated. |
94+
| operator.tolerations | list | `[]` | Tolerations for the operator pod |
95+
| operator.toolhiveRunnerImage | string | `"ghcr.io/stacklok/toolhive/proxyrunner:v0.6.5"` | Image to use for Toolhive runners |
96+
| operator.volumeMounts | list | `[]` | Additional volume mounts on the operator container |
97+
| operator.volumes | list | `[]` | Additional volumes to mount on the operator pod |
98+
| registryAPI | object | `{"image":"ghcr.io/stacklok/thv-registry-api:v0.1.0","serviceAccount":{"annotations":{},"automountServiceAccountToken":true,"labels":{},"name":"toolhive-registry-api"}}` | All values for the registry API deployment and associated resources |
99+
| registryAPI.image | string | `"ghcr.io/stacklok/thv-registry-api:v0.1.0"` | Container image for the registry API |
100+
| registryAPI.serviceAccount | object | `{"annotations":{},"automountServiceAccountToken":true,"labels":{},"name":"toolhive-registry-api"}` | Service account configuration for the registry API |
101+
| registryAPI.serviceAccount.annotations | object | `{}` | Annotations to add to the registry API service account |
102+
| registryAPI.serviceAccount.automountServiceAccountToken | bool | `true` | Automatically mount a ServiceAccount's API credentials |
103+
| registryAPI.serviceAccount.labels | object | `{}` | Labels to add to the registry API service account |
104+
| registryAPI.serviceAccount.name | string | `"toolhive-registry-api"` | The name of the service account to use for the registry API |
105+

‎docs/operator/composite-tools-quick-reference.md‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,12 @@ spec:
1717
failureMode: abort # Optional: abort|continue|best_effort (default: abort)
1818

1919
parameters: # Optional: input parameters
20-
param_name:
21-
type: string
22-
description: Description of the parameter
23-
required: false
20+
type: object
21+
properties:
22+
param_name:
23+
type: string
24+
description: Description of the parameter
25+
required: []
2426

2527
steps: # Required: workflow steps
2628
- id: step1

0 commit comments

Comments
 (0)