Skip to content

Conversation

@dmartinol
Copy link
Collaborator

@dmartinol dmartinol commented Nov 6, 2025

Complete changes requested for stacklok/toolhive#42

  • Add coordinator package to run background syncs with data lock concurrent access checks
  • Add File dats source with examples (closes Use x/exp/jsonrpc2 instead of our own implementation. toolhive#74 )
  • Add StatusPersistence interface with default persistence on file (./data/status.json)
  • Remove legacy configmap registry provider
  • Remove unneeded CLI flags: only --config remains

After this we can start:

  1. Integrating the conversion functions Integrate conversion functions #76
  2. Integrating the DB persistence Add Pushover MCP toolhive#32
  3. Start controller cleanup Registry Server Deployment toolhive#2396, Registry controller Cleanup toolhive#2399
$ ./bin/thv-registry-api serve --help                           
Start the registry API server to serve MCP registry data.

The server requires a configuration file (--config) that specifies:
- Registry name and data source (Git, ConfigMap, API, or File)
- Sync policy and filtering rules
- All other operational settings

See examples/ directory for sample configurations.

Usage:
  thv-registry-api serve [flags]

Flags:
      --address string   Address to listen on (default ":8080")
      --config string    Path to configuration file (YAML format, required)
  -h, --help             help for serve

Global Flags:
      --debug   Enable debug mode

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2025

Codecov Report

❌ Patch coverage is 34.15560% with 347 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.34%. Comparing base (ed6471b) to head (1994423).

Files with missing lines Patch % Lines
pkg/app/builder.go 13.75% 138 Missing ⚠️
pkg/sync/coordinator/coordinator.go 5.55% 68 Missing ⚠️
pkg/sync/mocks/mock_manager.go 0.00% 33 Missing ⚠️
pkg/app/app.go 0.00% 24 Missing ⚠️
pkg/sync/coordinator/sync.go 66.66% 14 Missing and 3 partials ⚠️
pkg/app/kubernetes.go 0.00% 16 Missing ⚠️
pkg/sources/storage_manager.go 65.71% 7 Missing and 5 partials ⚠️
pkg/status/persistence.go 55.55% 7 Missing and 5 partials ⚠️
pkg/config/config.go 58.33% 10 Missing ⚠️
pkg/sync/manager.go 64.70% 5 Missing and 1 partial ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main      stacklok/toolhive#77      +/-   ##
==========================================
- Coverage   65.91%   59.34%   -6.58%     
==========================================
  Files          35       44       +9     
  Lines        2224     2595     +371     
==========================================
+ Hits         1466     1540      +74     
- Misses        641      937     +296     
- Partials      117      118       +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.

Copy link

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 refactors the synchronization system to introduce a background sync coordinator that handles automatic registry synchronization. The key changes include:

  • Removed controller-runtime return values and timing logic from sync manager in favor of coordinator-based scheduling
  • Added new pkg/sync/coordinator package for background sync orchestration with ticker-based periodic syncs
  • Introduced file-based storage and status persistence to replace Kubernetes-specific storage
  • Added file source handler for local filesystem data sources
  • Updated Manager interface to remove ctrl.Result return and add manualSyncRequested parameter

Reviewed Changes

Copilot reviewed 31 out of 32 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
pkg/sync/manager.go Simplified sync manager by removing requeue timing logic and controller-runtime dependencies
pkg/sync/mocks/mock_manager.go Generated mock for updated Manager interface
pkg/sync/coordinator/*.go New coordinator package for background sync scheduling and status management
pkg/sources/storage_manager.go Implemented file-based storage manager replacing Kubernetes ConfigMap storage
pkg/sources/file.go New file source handler for local filesystem data sources
pkg/config/config.go Added file source configuration and validation
pkg/status/persistence.go New status persistence layer for sync state tracking
cmd/thv-registry-api/app/serve.go Refactored to use coordinator pattern with automatic sync
examples/*.yaml Added comprehensive example configurations for all source types

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@JAORMX
Copy link
Contributor

JAORMX commented Nov 6, 2025

@claude please review this

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

great work! The biggest concern is that with this amount of content we'll have no way of validating and keeping it up-to-date. Some suggestions sprinkled inline.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
@dmartinol
Copy link
Collaborator Author

dmartinol commented Nov 6, 2025

@claude please review this

@JAORMX IIUC, the bot is disabled #10

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
@dmartinol dmartinol mentioned this pull request Nov 6, 2025
4 tasks
Copy link
Contributor

@blkt blkt left a comment

Choose a reason for hiding this comment

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

I'm still going over the PR, here are a first batch of questions.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
blkt
blkt previously approved these changes Nov 7, 2025
Copy link
Contributor

@blkt blkt left a comment

Choose a reason for hiding this comment

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

LGTM, and big thanks for doing this.

One thing I ask of you is to please split these PRs into multiple ones in the future. For example, this one had the following areas of concern

  • refactored application startup introducing pkg/app
  • introduced persistence layer, which I believe @dmjb was working on as well
  • introduced pkg/sync, which is a big beast in and of itself

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
@dmartinol dmartinol merged commit f6cad56 into stacklok:main Nov 7, 2025
19 checks passed
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.

Use x/exp/jsonrpc2 instead of our own implementation.

6 participants