Add PVC source support for MCPRegistry #2719

jonburdo · 2025-11-25T03:40:26Z

Enables MCPRegistry resources to use PersistentVolumeClaims as a data source in addition to existing ConfigMap, Git, and API sources.

🤖 Generated with Claude Code

codecov · 2025-11-25T07:33:30Z

Codecov Report

❌ Patch coverage is 52.63158% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.58%. Comparing base (7e2a5e7) to head (7e4f733).

Files with missing lines	Patch %	Lines
cmd/thv-operator/pkg/registryapi/manager.go	0.00%	17 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2719      +/-   ##
==========================================
- Coverage   55.59%   55.58%   -0.01%     
==========================================
  Files         314      314              
  Lines       30445    30483      +38     
==========================================
+ Hits        16925    16945      +20     
- Misses      12033    12050      +17     
- Partials     1487     1488       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dmartinol

Thanks for your contribution!
I commented with some concerns about using the configured values, please TAL!

dmartinol · 2025-11-25T07:47:34Z

cmd/thv-operator/api/v1alpha1/mcpregistry_types.go

+	if r.Spec.Source.PVCRef == nil {
+		return "registry.json"
+	}
+
+	if r.Spec.Source.PVCRef.Path == "" {
+		return "registry.json"
+	}


can we collapse these to a single if check?

if r.Spec.Source.PVCRef == nil || r.Spec.Source.PVCRef.Path == ""

dmartinol · 2025-11-25T07:50:56Z

cmd/thv-operator/pkg/registryapi/config/config.go

+		// this stops the registry server worrying about PVC sources when all it has to do
+		// is read the file on startup
+		sourceConfig.File = &FileConfig{
+			Path: filepath.Join(RegistryJSONFilePath, RegistryJSONFileName),


instead of the constant RegistryJSONFileName, shouldn't we return the configured value at registrySpec.PvvcRef.Key?
in case, pls add a proper test to validate

BTW: I see that the same constant is returned when a ConfigMap is defined , we need to track with an issue and fix

I've added #2749

dmartinol · 2025-11-25T07:53:20Z

cmd/thv-operator/pkg/registryapi/config/config_test.go

+		// The custom path is handled by the volume mount, not the config
+		assert.Equal(t, SourceTypeFile, config.Source.Type)
+		require.NotNil(t, config.Source.File)
+		assert.Equal(t, filepath.Join(RegistryJSONFilePath, RegistryJSONFileName), config.Source.File.Path)


this should instead return the custom path custom/path/registry.json

dmartinol · 2025-11-25T07:54:40Z

examples/operator/mcp-registries/mcpregistry-pvc.yaml

+# Example Job to populate the PVC with registry data
+# This job can be run once to initialize the PVC, or periodically to update it
+apiVersion: batch/v1
+kind: Job


good job 👍

dmartinol · 2025-11-25T11:51:29Z

Need some adjustment to adapt to the new model:

naming review
multiple registries (FKA source)
The latter implies that the mounted path should use the regisstry name to create a sub-folder.
Finally, it would be great if you can extend the existing IT with this use case (maybe having a config with both ConfigMap and PVC)

Enables MCPRegistry resources to use PersistentVolumeClaims as a data source in addition to existing ConfigMap, Git, and API sources. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jon Burdo <[email protected]>

dmartinol · 2025-11-27T06:15:18Z

cmd/thv-operator/api/v1alpha1/mcpregistry_types.go

+
+	// RegistrySourceTypePVC is the type for registry data stored in PersistentVolumeClaims
+	RegistrySourceTypePVC = "pvc"
 )


This constant and also the others above can be safely removed IMO (missed to drop them in another cleanup task): can you pls remove all the const block?

dmartinol · 2025-11-27T06:19:30Z

cmd/thv-operator/pkg/registryapi/config/config.go

+		pvcPath := RegistryJSONFileName
+		if registrySpec.PVCRef.Path != "" {
+			pvcPath = registrySpec.PVCRef.Path
+		}


Try using the buildFilePath function instead, to include the registry name in the path (otherwise we could have duplicated file paths in the server config)

dmartinol · 2025-11-27T06:20:19Z

cmd/thv-operator/pkg/registryapi/config/config_test.go

+		assert.Equal(t, mcpv1alpha1.RegistryFormatToolHive, config.Registries[0].Format)
+		require.NotNil(t, config.Registries[0].File)
+		// Path defaults to registry.json at PVC root
+		assert.Equal(t, filepath.Join(RegistryJSONFilePath, RegistryJSONFileName), config.Registries[0].File.Path)


after the above change, the expected path includes the registry name

dmartinol

Please take a look at the REGISTRY.md file to include the new registry type (BTW: if you can, please also replace the "data sources" to "registries", it's another leftover of previous PRs

dmartinol reviewed Nov 25, 2025

View reviewed changes

github-actions bot added the size/L Large PR: 600-999 lines changed label Nov 25, 2025

jonburdo marked this pull request as draft November 26, 2025 03:27

jonburdo mentioned this pull request Nov 26, 2025

Use correct path for ConfigMap registry #2749

Open

jonburdo force-pushed the pvc-source branch 2 times, most recently from 822def6 to 27c80e2 Compare November 27, 2025 00:25

jonburdo force-pushed the pvc-source branch from 27c80e2 to 632f5b0 Compare November 27, 2025 03:59

jonburdo marked this pull request as ready for review November 27, 2025 04:02

dmartinol reviewed Nov 27, 2025

View reviewed changes

Add PVC source support for MCPRegistry #2719

Are you sure you want to change the base?

Add PVC source support for MCPRegistry #2719

Uh oh!

Conversation

jonburdo commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmartinol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmartinol commented Nov 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmartinol left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonburdo commented Nov 25, 2025 •

edited

Loading

codecov bot commented Nov 25, 2025 •

edited

Loading