diff --git a/internal/sync/coordinator/coordinator_test.go b/internal/sync/coordinator/coordinator_test.go index e1d5fa2..41d60d6 100644 --- a/internal/sync/coordinator/coordinator_test.go +++ b/internal/sync/coordinator/coordinator_test.go @@ -526,12 +526,12 @@ func TestUpdateStatusForSkippedSync(t *testing.T) { // Call updateStatusForSkippedSync reason := sync.ReasonUpToDateWithPolicy - coord.updateStatusForSkippedSync(context.Background(), regCfg, reason) + coord.updateStatusForSkippedSync(context.Background(), regCfg, reason.String()) // Verify status is updated but phase remains the same finalStatus := coord.cachedStatuses[registryName] assert.Equal(t, status.SyncPhaseComplete, finalStatus.Phase, "Phase should remain Complete") - assert.Contains(t, finalStatus.Message, reason, "Message should include skip reason") + assert.Contains(t, finalStatus.Message, reason.String(), "Message should include skip reason") // Verify status was persisted loadedStatus, err := statusPersistence.LoadStatus(context.Background(), registryName) @@ -557,10 +557,10 @@ func TestCheckRegistrySync_PerformsSyncWhenNeeded(t *testing.T) { } regCfg := &cfg.Registries[0] - // Mock ShouldSync to return true + // Mock ShouldSync to return a reason that requires sync mockSyncMgr.EXPECT(). ShouldSync(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Return(true, sync.ReasonSourceDataChanged, (*time.Time)(nil)) + Return(sync.ReasonSourceDataChanged) // Mock PerformSync mockSyncMgr.EXPECT(). @@ -610,10 +610,10 @@ func TestCheckRegistrySync_SkipsSyncWhenNotNeeded(t *testing.T) { } regCfg := &cfg.Registries[0] - // Mock ShouldSync to return false + // Mock ShouldSync to return a reason that does NOT require sync mockSyncMgr.EXPECT(). ShouldSync(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Return(false, sync.ReasonUpToDateWithPolicy, (*time.Time)(nil)) + Return(sync.ReasonUpToDateWithPolicy) // PerformSync should NOT be called mockSyncMgr.EXPECT(). diff --git a/internal/sync/coordinator/sync.go b/internal/sync/coordinator/sync.go index 95fa048..6206e00 100644 --- a/internal/sync/coordinator/sync.go +++ b/internal/sync/coordinator/sync.go @@ -9,6 +9,7 @@ import ( "github.com/stacklok/toolhive-registry-server/internal/config" "github.com/stacklok/toolhive-registry-server/internal/status" + "github.com/stacklok/toolhive-registry-server/internal/sync" ) // checkRegistrySync performs a sync check and updates status accordingly for a specific registry @@ -16,18 +17,20 @@ func (c *defaultCoordinator) checkRegistrySync(ctx context.Context, regCfg *conf registryName := regCfg.Name // Check if sync is needed (read status under lock) - // ShouldSync will return false with ReasonAlreadyInProgress if Phase == SyncPhaseSyncing - var shouldSync bool - var reason string + // ShouldSync will return ReasonAlreadyInProgress if Phase == SyncPhaseSyncing + var reason sync.Reason c.withRegistryStatus(registryName, func(syncStatus *status.SyncStatus) { - shouldSync, reason, _ = c.manager.ShouldSync(ctx, regCfg, syncStatus, false) + reason = c.manager.ShouldSync(ctx, regCfg, syncStatus, false) }) - logger.Infof("Registry '%s': %s sync check: shouldSync=%v, reason=%s", registryName, checkType, shouldSync, reason) + logger.Infof( + "Registry '%s': %s sync check: shouldSync=%v, reason=%s", + registryName, checkType, reason.ShouldSync(), reason.String(), + ) - if shouldSync { + if reason.ShouldSync() { c.performRegistrySync(ctx, regCfg) } else { - c.updateStatusForSkippedSync(ctx, regCfg, reason) + c.updateStatusForSkippedSync(ctx, regCfg, reason.String()) } } diff --git a/internal/sync/doc.go b/internal/sync/doc.go index cb0508a..dbd49ba 100644 --- a/internal/sync/doc.go +++ b/internal/sync/doc.go @@ -37,17 +37,23 @@ // // # Sync Reasons // -// The package defines extensive reason constants to track why syncs occur or don't: +// The Manager.ShouldSync method returns a Reason enum that encodes both +// whether sync is needed and the reason. Use Reason.ShouldSync() to check +// if sync is needed and Reason.String() to get the reason string. // +// Sync reasons that indicate sync is NOT needed: // - ReasonAlreadyInProgress: Sync already running +// - ReasonManualNoChanges: Manual sync requested but no changes detected +// - ReasonErrorCheckingSyncNeed: Error checking if sync is needed +// - ReasonUpToDateWithPolicy: No sync needed, data is current with policy +// - ReasonUpToDateNoPolicy: No sync needed, no automatic policy +// +// Sync reasons that indicate sync IS needed: // - ReasonRegistryNotReady: Initial sync or recovery from failure -// - ReasonSourceDataChanged: Source data hash changed // - ReasonFilterChanged: Filter configuration modified +// - ReasonSourceDataChanged: Source data hash changed +// - ReasonErrorCheckingChanges: Error during change detection, sync anyway // - ReasonManualWithChanges: Manual sync requested with detected changes -// - ReasonManualNoChanges: Manual sync requested but no changes detected -// - ReasonUpToDateWithPolicy: No sync needed, data is current -// - ReasonUpToDateNoPolicy: No sync needed, no automatic policy -// - ReasonErrorCheckingChanges: Error during change detection // // # Kubernetes Status Integration // diff --git a/internal/sync/manager.go b/internal/sync/manager.go index 8ddec0d..4add91f 100644 --- a/internal/sync/manager.go +++ b/internal/sync/manager.go @@ -22,31 +22,61 @@ type Result struct { ServerCount int } -// Sync reason constants -const ( - // Registry state related reasons - ReasonAlreadyInProgress = "sync-already-in-progress" - ReasonRegistryNotReady = "registry-not-ready" - - // Filter change related reasons - ReasonFilterChanged = "filter-changed" - - // Data change related reasons - ReasonSourceDataChanged = "source-data-changed" - ReasonErrorCheckingChanges = "error-checking-data-changes" +// Reason represents the decision and reason for whether a sync should occur +type Reason int - // Manual sync related reasons - ReasonManualWithChanges = "manual-sync-with-data-changes" - ReasonManualNoChanges = "manual-sync-no-data-changes" +//nolint:revive // Group comments are for categorization, not per-constant documentation +const ( + // Reasons that require sync + ReasonRegistryNotReady Reason = iota + ReasonFilterChanged + ReasonSourceDataChanged + ReasonErrorCheckingChanges + ReasonManualWithChanges + + // Reasons that do NOT require sync + ReasonAlreadyInProgress + ReasonManualNoChanges + ReasonErrorParsingInterval + ReasonErrorCheckingSyncNeed + ReasonUpToDateWithPolicy + ReasonUpToDateNoPolicy +) - // Automatic sync related reasons - ReasonErrorParsingInterval = "error-parsing-sync-interval" - ReasonErrorCheckingSyncNeed = "error-checking-sync-need" +// String returns the string representation of the sync reason +func (r Reason) String() string { + switch r { + case ReasonRegistryNotReady: + return "registry-not-ready" + case ReasonFilterChanged: + return "filter-changed" + case ReasonSourceDataChanged: + return "source-data-changed" + case ReasonErrorCheckingChanges: + return "error-checking-data-changes" + case ReasonManualWithChanges: + return "manual-sync-with-data-changes" + case ReasonAlreadyInProgress: + return "sync-already-in-progress" + case ReasonManualNoChanges: + return "manual-sync-no-data-changes" + case ReasonErrorParsingInterval: + return "error-parsing-sync-interval" + case ReasonErrorCheckingSyncNeed: + return "error-checking-sync-need" + case ReasonUpToDateWithPolicy: + return "up-to-date-with-policy" + case ReasonUpToDateNoPolicy: + return "up-to-date-no-policy" + default: + return "unknown-sync-reason" + } +} - // Up-to-date reasons - ReasonUpToDateWithPolicy = "up-to-date-with-policy" - ReasonUpToDateNoPolicy = "up-to-date-no-policy" -) +// ShouldSync returns true if sync is needed, false otherwise +func (r Reason) ShouldSync() bool { + return r <= ReasonManualWithChanges +} // Manual sync annotation detection reasons const ( @@ -101,9 +131,10 @@ func (e *Error) Unwrap() error { //go:generate mockgen -destination=mocks/mock_manager.go -package=mocks github.com/stacklok/toolhive-registry-server/internal/sync Manager type Manager interface { // ShouldSync determines if a sync operation is needed for a specific registry + // Returns the sync reason which encodes both whether sync is needed and the reason ShouldSync( ctx context.Context, regCfg *config.RegistryConfig, syncStatus *status.SyncStatus, manualSyncRequested bool, - ) (bool, string, *time.Time) + ) Reason // PerformSync executes the complete sync operation for a specific registry PerformSync(ctx context.Context, regCfg *config.RegistryConfig) (*Result, *Error) @@ -147,19 +178,18 @@ func NewDefaultSyncManager( } // ShouldSync determines if a sync operation is needed for a specific registry -// Returns: (shouldSync bool, reason string, nextSyncTime *time.Time) -// nextSyncTime is always nil - timing is controlled by the configured sync interval +// Returns a Reason which encodes both whether sync is needed and the reason func (s *defaultSyncManager) ShouldSync( ctx context.Context, regCfg *config.RegistryConfig, syncStatus *status.SyncStatus, manualSyncRequested bool, -) (bool, string, *time.Time) { +) Reason { ctxLogger := log.FromContext(ctx) // If registry is currently syncing, don't start another sync if syncStatus.Phase == status.SyncPhaseSyncing { - return false, ReasonAlreadyInProgress, nil + return ReasonAlreadyInProgress } // Check if sync is needed based on registry state @@ -170,10 +200,9 @@ func (s *defaultSyncManager) ShouldSync( checkIntervalElapsed, _, err := s.automaticSyncChecker.IsIntervalSyncNeeded(regCfg, syncStatus) if err != nil { ctxLogger.Error(err, "Failed to determine if interval has elapsed") - return false, ReasonErrorCheckingSyncNeed, nil + return ReasonErrorCheckingSyncNeed } - shouldSync := false reason := ReasonUpToDateNoPolicy // Check if update is needed for state, manual sync, or filter change @@ -183,12 +212,10 @@ func (s *defaultSyncManager) ShouldSync( dataChanged, err := s.dataChangeDetector.IsDataChanged(ctx, regCfg, syncStatus) if err != nil { ctxLogger.Error(err, "Failed to determine if data has changed") - shouldSync = true reason = ReasonErrorCheckingChanges } else { ctxLogger.Info("Checked data changes", "dataChanged", dataChanged) if dataChanged { - shouldSync = true if syncNeededForState { reason = ReasonRegistryNotReady } else if manualSyncRequested { @@ -199,7 +226,6 @@ func (s *defaultSyncManager) ShouldSync( reason = ReasonSourceDataChanged } } else { - shouldSync = false if manualSyncRequested { reason = ReasonManualNoChanges } @@ -210,9 +236,9 @@ func (s *defaultSyncManager) ShouldSync( ctxLogger.Info("ShouldSync", "syncNeededForState", syncNeededForState, "filterChanged", filterChanged, "manualSyncRequested", manualSyncRequested, "checkIntervalElapsed", checkIntervalElapsed, "dataChanged", dataChangedString) - ctxLogger.Info("ShouldSync returning", "shouldSync", shouldSync, "reason", reason) + ctxLogger.Info("ShouldSync returning", "reason", reason.String(), "shouldSync", reason.ShouldSync()) - return shouldSync, reason, nil + return reason } // isSyncNeededForState checks if sync is needed based on the registry's current state diff --git a/internal/sync/manager_additional_test.go b/internal/sync/manager_additional_test.go index de6109c..bd4f093 100644 --- a/internal/sync/manager_additional_test.go +++ b/internal/sync/manager_additional_test.go @@ -106,20 +106,36 @@ func TestResult_ZeroValues(t *testing.T) { assert.Equal(t, 0, result.ServerCount) } -func TestSyncReasonConstants(t *testing.T) { +func TestReasonConstants(t *testing.T) { t.Parallel() - // Verify sync reason constants are properly defined - assert.Equal(t, "sync-already-in-progress", ReasonAlreadyInProgress) - assert.Equal(t, "registry-not-ready", ReasonRegistryNotReady) - assert.Equal(t, "error-checking-sync-need", ReasonErrorCheckingSyncNeed) - assert.Equal(t, "error-checking-data-changes", ReasonErrorCheckingChanges) - assert.Equal(t, "error-parsing-sync-interval", ReasonErrorParsingInterval) - assert.Equal(t, "source-data-changed", ReasonSourceDataChanged) - assert.Equal(t, "manual-sync-with-data-changes", ReasonManualWithChanges) - assert.Equal(t, "manual-sync-no-data-changes", ReasonManualNoChanges) - assert.Equal(t, "up-to-date-with-policy", ReasonUpToDateWithPolicy) - assert.Equal(t, "up-to-date-no-policy", ReasonUpToDateNoPolicy) + // Verify sync reason constants are properly defined and have correct string representations + // Order matches the original const block + assert.Equal(t, "sync-already-in-progress", ReasonAlreadyInProgress.String()) + assert.Equal(t, "registry-not-ready", ReasonRegistryNotReady.String()) + assert.Equal(t, "filter-changed", ReasonFilterChanged.String()) + assert.Equal(t, "source-data-changed", ReasonSourceDataChanged.String()) + assert.Equal(t, "error-checking-data-changes", ReasonErrorCheckingChanges.String()) + assert.Equal(t, "manual-sync-with-data-changes", ReasonManualWithChanges.String()) + assert.Equal(t, "manual-sync-no-data-changes", ReasonManualNoChanges.String()) + assert.Equal(t, "error-parsing-sync-interval", ReasonErrorParsingInterval.String()) + assert.Equal(t, "error-checking-sync-need", ReasonErrorCheckingSyncNeed.String()) + assert.Equal(t, "up-to-date-with-policy", ReasonUpToDateWithPolicy.String()) + assert.Equal(t, "up-to-date-no-policy", ReasonUpToDateNoPolicy.String()) + + // Verify ShouldSync() returns correct values + assert.False(t, ReasonAlreadyInProgress.ShouldSync()) + assert.False(t, ReasonManualNoChanges.ShouldSync()) + assert.False(t, ReasonErrorParsingInterval.ShouldSync()) + assert.False(t, ReasonErrorCheckingSyncNeed.ShouldSync()) + assert.False(t, ReasonUpToDateWithPolicy.ShouldSync()) + assert.False(t, ReasonUpToDateNoPolicy.ShouldSync()) + + assert.True(t, ReasonRegistryNotReady.ShouldSync()) + assert.True(t, ReasonFilterChanged.ShouldSync()) + assert.True(t, ReasonSourceDataChanged.ShouldSync()) + assert.True(t, ReasonErrorCheckingChanges.ShouldSync()) + assert.True(t, ReasonManualWithChanges.ShouldSync()) } func TestConditionReasonConstants(t *testing.T) { diff --git a/internal/sync/manager_test.go b/internal/sync/manager_test.go index c84b129..3b5e520 100644 --- a/internal/sync/manager_test.go +++ b/internal/sync/manager_test.go @@ -51,9 +51,7 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { manualSyncRequested bool config *config.RegistryConfig syncStatus *status.SyncStatus - expectedSyncNeeded bool - expectedReason string - expectedNextTime bool // whether nextSyncTime should be set + expectedReason Reason }{ { name: "sync needed when registry is in failed state", @@ -68,9 +66,7 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { syncStatus: &status.SyncStatus{ Phase: status.SyncPhaseFailed, }, - expectedSyncNeeded: true, - expectedReason: ReasonRegistryNotReady, - expectedNextTime: false, + expectedReason: ReasonRegistryNotReady, }, { name: "sync not needed when already syncing", @@ -85,9 +81,7 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { syncStatus: &status.SyncStatus{ Phase: status.SyncPhaseSyncing, }, - expectedSyncNeeded: false, - expectedReason: ReasonAlreadyInProgress, - expectedNextTime: false, + expectedReason: ReasonAlreadyInProgress, }, { name: "sync needed when registry is in failed state", @@ -103,9 +97,7 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { Phase: status.SyncPhaseFailed, LastSyncHash: "", }, - expectedSyncNeeded: true, - expectedReason: ReasonRegistryNotReady, - expectedNextTime: false, + expectedReason: ReasonRegistryNotReady, }, { name: "manual sync not needed with new trigger value and same hash", @@ -121,9 +113,7 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { Phase: status.SyncPhaseComplete, LastSyncHash: testHash, }, - expectedSyncNeeded: false, - expectedReason: ReasonManualNoChanges, // No data changes but manual trigger - expectedNextTime: false, + expectedReason: ReasonManualNoChanges, // No data changes but manual trigger }, } @@ -138,22 +128,11 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - syncNeeded, reason, nextSyncTime := syncManager.ShouldSync(ctx, tt.config, tt.syncStatus, tt.manualSyncRequested) + reason := syncManager.ShouldSync(ctx, tt.config, tt.syncStatus, tt.manualSyncRequested) - // We expect some errors for file source operations, but that's okay for this test - if tt.expectedSyncNeeded { - assert.True(t, syncNeeded, "Expected sync to be needed for "+tt.name) - assert.Equal(t, tt.expectedReason, reason, "Expected specific sync reason") - } else { - assert.False(t, syncNeeded, "Expected sync not to be needed for "+tt.name) - assert.Equal(t, tt.expectedReason, reason, "Expected specific sync reason") - } - - if tt.expectedNextTime { - assert.NotNil(t, nextSyncTime, "Expected next sync time to be set") - } else { - assert.Nil(t, nextSyncTime, "Expected next sync time to be nil") - } + assert.Equal(t, tt.expectedReason, reason, "Expected specific sync reason for "+tt.name) + assert.Equal(t, tt.expectedReason.ShouldSync(), reason.ShouldSync(), "ShouldSync() should match expected reason") + assert.Equal(t, tt.expectedReason.String(), reason.String(), "String() should match expected reason") }) } } @@ -325,7 +304,7 @@ func TestIsManualSync(t *testing.T) { t.Parallel() tests := []struct { - reason string + reason Reason expected bool }{ {ReasonManualWithChanges, true}, @@ -336,7 +315,7 @@ func TestIsManualSync(t *testing.T) { } for _, tt := range tests { - t.Run(tt.reason, func(t *testing.T) { + t.Run(tt.reason.String(), func(t *testing.T) { t.Parallel() result := IsManualSync(tt.reason) assert.Equal(t, tt.expected, result) diff --git a/internal/sync/mocks/mock_manager.go b/internal/sync/mocks/mock_manager.go index 5d39130..a91a161 100644 --- a/internal/sync/mocks/mock_manager.go +++ b/internal/sync/mocks/mock_manager.go @@ -12,7 +12,6 @@ package mocks import ( context "context" reflect "reflect" - time "time" config "github.com/stacklok/toolhive-registry-server/internal/config" status "github.com/stacklok/toolhive-registry-server/internal/status" @@ -74,13 +73,11 @@ func (mr *MockManagerMockRecorder) PerformSync(ctx, regCfg any) *gomock.Call { } // ShouldSync mocks base method. -func (m *MockManager) ShouldSync(ctx context.Context, regCfg *config.RegistryConfig, syncStatus *status.SyncStatus, manualSyncRequested bool) (bool, string, *time.Time) { +func (m *MockManager) ShouldSync(ctx context.Context, regCfg *config.RegistryConfig, syncStatus *status.SyncStatus, manualSyncRequested bool) sync.Reason { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ShouldSync", ctx, regCfg, syncStatus, manualSyncRequested) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(string) - ret2, _ := ret[2].(*time.Time) - return ret0, ret1, ret2 + ret0, _ := ret[0].(sync.Reason) + return ret0 } // ShouldSync indicates an expected call of ShouldSync. diff --git a/internal/sync/utils.go b/internal/sync/utils.go index 3abad30..35c573b 100644 --- a/internal/sync/utils.go +++ b/internal/sync/utils.go @@ -1,6 +1,6 @@ package sync // IsManualSync checks if the sync reason indicates a manual sync -func IsManualSync(reason string) bool { +func IsManualSync(reason Reason) bool { return reason == ReasonManualWithChanges || reason == ReasonManualNoChanges }