Improve ShouldSync return type

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().

internal/sync/coordinator/sync.go

@@ -9,25 +9,28 @@ 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
 func (c *defaultCoordinator) checkRegistrySync(ctx context.Context, regCfg *config.RegistryConfig, checkType string) {
 	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())
 	}
 }
 

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
 //

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

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) {