Skip to content

Conversation

dmartinol
Copy link
Collaborator

Fixes #2104

Changes

  • Added Registry API source handler with type: api
  • Added UT and IT
  • Updated docs

Postponed requirements

  • Management of upstream API and schema. Will track a separate issue for that

Copy link

codecov bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 83.51648% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.47%. Comparing base (ffd3414) to head (b977f65).

Files with missing lines Patch % Lines
cmd/thv-operator/pkg/sources/api_toolhive.go 79.80% 31 Missing and 10 partials ⚠️
cmd/thv-operator/pkg/sources/api.go 79.22% 14 Missing and 2 partials ⚠️
cmd/thv-operator/pkg/httpclient/client.go 91.17% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2189      +/-   ##
==========================================
+ Coverage   53.09%   53.47%   +0.37%     
==========================================
  Files         222      227       +5     
  Lines       28904    29268     +364     
==========================================
+ Hits        15347    15650     +303     
- Misses      12419    12467      +48     
- Partials     1138     1151      +13     

☔ 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.

@jhrozek jhrozek requested a review from Copilot October 15, 2025 11:09
@jhrozek
Copy link
Contributor

jhrozek commented Oct 15, 2025

@claude provide a review. During the review, you must take into account the existing flows of the operator and how it synchronizes its data sources.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for fetching MCP registry data from API endpoints, enabling dynamic synchronization from remote registries via HTTP/HTTPS.

Key Changes:

  • Added API source handler with automatic format detection (ToolHive vs Upstream MCP Registry)
  • Implemented HTTP client for API operations with timeout and error handling
  • Added comprehensive unit and integration tests for API source functionality

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
cmd/thv-operator/api/v1alpha1/mcpregistry_types.go Added APISource type and RegistrySourceTypeAPI constant to support API-based registry sources
cmd/thv-operator/pkg/sources/api.go Main API source handler with format detection and delegation logic
cmd/thv-operator/pkg/sources/api_toolhive.go ToolHive-specific API handler for fetching and converting registry data
cmd/thv-operator/pkg/sources/api_upstream.go Upstream MCP Registry API handler (validation only, Phase 2 not implemented)
cmd/thv-operator/pkg/sources/api_types.go Type aliases for ToolHive Registry API response types
cmd/thv-operator/pkg/httpclient/client.go HTTP client implementation with context support and standard headers
cmd/thv-operator/pkg/httpclient/types.go HTTPError type for structured error handling
cmd/thv-operator/pkg/sources/factory.go Factory updated to create API source handlers
deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml CRD updated with API source configuration
docs/operator/crd-api.md Documentation for API source configuration
cmd/thv-operator/REGISTRY.md User guide for API source configuration
examples/operator/mcp-registries/*.yaml Example configurations for API source usage

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +140 to +142
// - /v1/registry/servers - List all servers (single response, no pagination)
// - /v1/registry/servers/{name} - Get specific server (future)
// - /v1/registry/info - Get registry metadata (future)
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API paths in the documentation use /v1/registry/* but the actual implementation in api_toolhive.go uses /v0/* paths (e.g., /v0/info, /v0/servers). Update the documentation to match the implementation.

Suggested change
// - /v1/registry/servers - List all servers (single response, no pagination)
// - /v1/registry/servers/{name} - Get specific server (future)
// - /v1/registry/info - Get registry metadata (future)
// - /v0/servers - List all servers (single response, no pagination)
// - /v0/servers/{name} - Get specific server (future)
// - /v0/info - Get registry metadata (future)

Copilot uses AI. Check for mistakes.


| Field | Description | Default | Validation |
| --- | --- | --- | --- |
| `endpoint` _string_ | Endpoint is the base API URL (without path)<br />The controller will append the appropriate paths:<br />Phase 1 (ToolHive API):<br /> - /v1/registry/servers - List all servers (single response, no pagination)<br /> - /v1/registry/servers/\{name\} - Get specific server (future)<br /> - /v1/registry/info - Get registry metadata (future)<br />Phase 2 (Upstream MCP Registry API):<br /> - /v0/servers - List all servers with pagination<br /> - /v0/servers/\{serverName\}/versions - List server versions<br />Example: "http://my-registry-api.default.svc.cluster.local/api" | | MinLength: 1 <br />Pattern: `^https?://.*` <br />Required: \{\} <br /> |
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API paths in the documentation use /v1/registry/* for ToolHive API but the implementation uses /v0/* paths. Update the documentation to match the implementation's use of /v0/info, /v0/servers, etc.

Suggested change
| `endpoint` _string_ | Endpoint is the base API URL (without path)<br />The controller will append the appropriate paths:<br />Phase 1 (ToolHive API):<br /> - /v1/registry/servers - List all servers (single response, no pagination)<br /> - /v1/registry/servers/\{name\} - Get specific server (future)<br /> - /v1/registry/info - Get registry metadata (future)<br />Phase 2 (Upstream MCP Registry API):<br /> - /v0/servers - List all servers with pagination<br /> - /v0/servers/\{serverName\}/versions - List server versions<br />Example: "http://my-registry-api.default.svc.cluster.local/api" | | MinLength: 1 <br />Pattern: `^https?://.*` <br />Required: \{\} <br /> |
| `endpoint` _string_ | Endpoint is the base API URL (without path)<br />The controller will append the appropriate paths:<br />Phase 1 (ToolHive API):<br /> - /v0/servers - List all servers (single response, no pagination)<br /> - /v0/servers/\{name\} - Get specific server (future)<br /> - /v0/info - Get registry metadata (future)<br />Phase 2 (Upstream MCP Registry API):<br /> - /v0/servers - List all servers with pagination<br /> - /v0/servers/\{serverName\}/versions - List server versions<br />Example: "http://my-registry-api.default.svc.cluster.local/api" | | MinLength: 1 <br />Pattern: `^https?://.*` <br />Required: \{\} <br /> |

Copilot uses AI. Check for mistakes.

Comment on lines +121 to +126
Phase 1 (ToolHive API):
- /v1/registry/servers - List all servers (single response, no pagination)
- /v1/registry/servers/{name} - Get specific server (future)
- /v1/registry/info - Get registry metadata (future)
Phase 2 (Upstream MCP Registry API):
- /v0/servers - List all servers with pagination
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CRD documentation references /v1/registry/* paths for ToolHive API, but the implementation uses /v0/* paths. Update to match the actual implementation.

Suggested change
Phase 1 (ToolHive API):
- /v1/registry/servers - List all servers (single response, no pagination)
- /v1/registry/servers/{name} - Get specific server (future)
- /v1/registry/info - Get registry metadata (future)
Phase 2 (Upstream MCP Registry API):
- /v0/servers - List all servers with pagination
Supported API endpoints:
- /v0/servers - List all servers (with pagination)

Copilot uses AI. Check for mistakes.

type: api
api:
# External registry API endpoint via HTTPS
# Base URL - controller appends /v0/servers
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment says controller appends /v0/servers which is correct according to the implementation. However, the API type documentation incorrectly states /v1/registry/servers. This comment is accurate - no change needed here, but confirms the documentation issue elsewhere.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

claude bot commented Oct 15, 2025

Claude encountered an error —— View job

Failed with exit code 128

I'll analyze this and get back to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Kubernetes MCPRegistry] Registry API Data Source

2 participants