Skip to content

Conversation

@dmartinol
Copy link
Collaborator

Fixes #1743

  • Implemented Automatic and Manual sync policies
  • Refactored controller code to extract business logic in separate handlers

Automatic trigger accepts only the interval parameter for now, no retry policies:

  syncPolicy:
    interval: 1m

It is defined using Go duration format. Examples: "1h", "30m", "24h"

Manual annotation works with trigger annotations like:

kubectl annotate mcpregistry auto-sync-registry \                                
    toolhive.stacklok.dev/sync-trigger=NEW_VALUE --overwrite

Sync status is defined in the status secrtion:

  lastManualSyncTrigger: "6"
  lastSyncHash: b67513010ff3fbf6d0b36c9df3bb4ee54f237673a872356488652c849010a1b6
  lastSyncTime: "2025-09-15T08:15:23Z"

- ConfigMap source handler
- Basic unit tests

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
- Updated tests and implementation to replace hardcoded "registry.json" key with ConfigMapSourceDataKey constant.
- Introduced ConfigMapStorageManager for managing registry data in ConfigMaps, including methods for storing, retrieving, and deleting data.
- Added comprehensive unit tests for the new storage manager functionality.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
- Updated test cases to replace hardcoded "registry.json" key with ConfigMapStorageDataKey constant.
- Ensured consistency in data handling across storage manager methods.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
- Updated ConfigMapSourceHandler to validate and parse registry data directly, removing legacy methods.
- Enhanced test cases to utilize a new TestRegistryBuilder for generating test data, improving readability and maintainability.
- Introduced new methods for handling registry data in a more structured way, including validation and conversion between formats.
- Updated SyncResult to encapsulate parsed registry data instead of raw data, streamlining the sync process.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
- Updated the storage manager implementation and tests to replace the hardcoded "registry-storage" component label with the new RegistryStorageComponent constant.
- Improved consistency and maintainability in label handling across the storage manager.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
- Introduced sync operation checks to determine if a sync is necessary before execution.
- Enhanced the ConfigMapSourceHandler to fetch registry data and validate source configurations more effectively.
- Improved test cases to reflect changes in method names and data handling.

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
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 57.31707% with 140 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.45%. Comparing base (a39b1c8) to head (f4a2011).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...thv-operator/controllers/mcpregistry_controller.go 0.00% 75 Missing ⚠️
cmd/thv-operator/pkg/sync/manager.go 67.20% 49 Missing and 12 partials ⚠️
cmd/thv-operator/pkg/sync/detectors.go 94.00% 2 Missing and 1 partial ⚠️
cmd/thv-operator/pkg/sources/storage_manager.go 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1886      +/-   ##
==========================================
+ Coverage   45.80%   46.45%   +0.64%     
==========================================
  Files         203      206       +3     
  Lines       25979    26145     +166     
==========================================
+ Hits        11900    12145     +245     
+ Misses      13167    13075      -92     
- Partials      912      925      +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.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
jhrozek
jhrozek previously approved these changes Sep 15, 2025
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.

I left two nits that are OK to be fixed in a later PR

@dmartinol dmartinol requested a review from Copilot September 15, 2025 10:21
Copy link
Contributor

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 implements automatic and manual sync policies for MCPRegistry resources, addressing issue #1743. The implementation introduces a clean separation of concerns by extracting sync logic from the controller into dedicated interfaces and handlers.

  • Automatic sync supports interval-based triggering with configurable Go duration format (e.g., "1h", "30m", "24h")
  • Manual sync is triggered via toolhive.stacklok.dev/sync-trigger annotation with value tracking to prevent duplicate syncs
  • Comprehensive refactoring extracts business logic into separate sync package with proper interfaces and implementations

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
docs/operator/crd-api.md Adds documentation for new lastManualSyncTrigger status field
deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpregistries.yaml Updates CRD schema to include lastManualSyncTrigger field
deploy/charts/operator-crds/README.md Bumps chart version to 0.0.25
deploy/charts/operator-crds/Chart.yaml Updates chart version to 0.0.25
cmd/thv-operator/pkg/sync/ New sync package with interfaces and implementations for sync management
cmd/thv-operator/pkg/sources/storage_manager.go Improves error handling and storage manager implementation
cmd/thv-operator/controllers/mcpregistry_controller.go Refactored to use new sync manager interface
cmd/thv-operator/api/v1alpha1/mcpregistry_types.go Adds lastManualSyncTrigger field to status

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

@jhrozek
Copy link
Contributor

jhrozek commented Sep 15, 2025

@dmartinol if you end up pushing a new version then getting rid of the commented out code might be good as well

@coveralls
Copy link
Collaborator

coveralls commented Sep 15, 2025

Coverage Status

coverage: 43.639% (+0.6%) from 43.054%
when pulling f4a2011 on dmartinol:registry_sync
into a39b1c8 on stacklok:main.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
@jhrozek jhrozek merged commit 3bd85de into stacklok:main Sep 15, 2025
27 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.

[Kubernetes MCPRegistry] Sync Policy

3 participants