Review the design of Collector interface #2004
Description
The current Collector interface is indeed violating the Single Responsibility Principle and Interface Segregation Principle. It's aggregating too many concerns into one interface.
Current Issues
Looking at the interface, it handles:
- Sync-related status (SetSyncStatus)
- API-related status (SetAPIStatus, SetAPIReadyCondition)
- Overall status derivation (SetPhase, SetMessage)
- Persistence (Apply)
This creates several problems:
- Tight coupling: Components that only need sync updates still get the entire interface
- Testing complexity: Hard to test individual concerns in isolation
- Unclear responsibilities: One interface doing too many things
- Future maintenance: Adding new status types requires modifying this monolithic interface
Proposed Design
Here's a better separation of concerns:
// Domain-specific collectors
type SyncStatusCollector interface {
SetSyncStatus(phase SyncPhase, message string, attemptCount int,
lastSyncTime *metav1.Time, lastSyncHash string, serverCount int)
SetSyncCondition(condition metav1.Condition)
}
type APIStatusCollector interface {
SetAPIStatus(phase APIPhase, message string, endpoint string)
SetAPIReadyCondition(reason, message string, status metav1.ConditionStatus)
}
// Overall status derivation (could be a separate service)
type StatusDeriver interface {
DeriveOverallStatus(syncStatus *SyncStatus, apiStatus *APIStatus) (MCPRegistryPhase, string)
}
// Orchestrator that coordinates all status updates
type StatusManager interface {
Sync() SyncStatusCollector
API() APIStatusCollector
SetOverallStatus(phase MCPRegistryPhase, message string) // for explicit overrides
Apply(ctx context.Context, k8sClient client.Client) error
}Usage Example
// In sync manager
statusManager.Sync().SetSyncStatus(SyncPhaseComplete, "Sync successful", 0, &now, hash, 5)
// In API manager
statusManager.API().SetAPIStatus(APIPhaseReady, "API ready", endpoint)
statusManager.API().SetAPIReadyCondition("Ready", "API is ready", metav1.ConditionTrue)
// Auto-derive overall status or set explicitly
overallPhase, overallMessage := statusDeriver.DeriveOverallStatus(syncStatus, apiStatus)
statusManager.SetOverallStatus(overallPhase, overallMessage)
// Single apply at the end
statusManager.Apply(ctx, k8sClient)Benefits
- Better separation of concerns: Each collector has a focused responsibility
- Interface segregation: Components only depend on what they need
- Easier testing: Can mock individual collectors independently
- Clearer code: Intent is more obvious when calling statusManager.Sync().SetSyncStatus(...)
- Extensibility: Adding new status types (e.g., storage, security) doesn't affect existing interfaces
- Composition over inheritance: StatusManager composes smaller, focused interfaces
This design would make the code much more maintainable and follows SOLID principles better.