Refactor status collector architecture with structured errors (#2016)

* Refactor status collector architecture with structured errors

* added deriver to codespell ignore list

* more test coverage

Signed-off-by: Daniele Martinoli <[email protected]>

* review comment

Signed-off-by: Daniele Martinoli <[email protected]>

* Add MCPRegistry status management improvements

- Removed the `Idle` phase from `SyncPhase` and updated related logic to ensure only `Syncing`, `Complete`, and `Failed` phases are used.
- Enhanced the `ShouldSync` method to avoid syncing when the requeue timeout did not elapse.
- Avoid updating sync status at all if sync is not needed (it would cause a new reconciliation)

Signed-off-by: Daniele Martinoli <[email protected]>

* chart version bump

Signed-off-by: Daniele Martinoli <[email protected]>

* Update cmd/thv-operator/pkg/mcpregistrystatus/types.go

Co-authored-by: Copilot <[email protected]>

---------

Signed-off-by: Daniele Martinoli <[email protected]>
Co-authored-by: Copilot <[email protected]>

Author: dmartinol

.codespellrc

@@ -1,3 +1,3 @@
 [codespell]
-ignore-words-list = NotIn,notin,AfterAll,ND,aks
+ignore-words-list = NotIn,notin,AfterAll,ND,aks,deriver
 skip = *.svg,*.mod,*.sum

cmd/thv-operator/api/v1alpha1/mcpregistry_types.go

@@ -205,7 +205,7 @@ type MCPRegistryStatus struct {
 // SyncStatus provides detailed information about data synchronization
 type SyncStatus struct {
 	// Phase represents the current synchronization phase
-	// +kubebuilder:validation:Enum=Idle;Syncing;Complete;Failed
+	// +kubebuilder:validation:Enum=Syncing;Complete;Failed
 	Phase SyncPhase `json:"phase"`
 
 	// Message provides additional information about the sync status
@@ -256,13 +256,10 @@ type APIStatus struct {
 }
 
 // SyncPhase represents the data synchronization state
-// +kubebuilder:validation:Enum=Idle;Syncing;Complete;Failed
+// +kubebuilder:validation:Enum=Syncing;Complete;Failed
 type SyncPhase string
 
 const (
-	// SyncPhaseIdle means no sync is needed or scheduled
-	SyncPhaseIdle SyncPhase = "Idle"
-
 	// SyncPhaseSyncing means sync is currently in progress
 	SyncPhaseSyncing SyncPhase = "Syncing"
 
@@ -389,7 +386,7 @@ func (r *MCPRegistry) DeriveOverallPhase() MCPRegistryPhase {
 	apiStatus := r.Status.APIStatus
 
 	// Default phases if status not set
-	syncPhase := SyncPhaseIdle
+	var syncPhase SyncPhase
 	if syncStatus != nil {
 		syncPhase = syncStatus.Phase
 	}
@@ -409,8 +406,8 @@ func (r *MCPRegistry) DeriveOverallPhase() MCPRegistryPhase {
 		return MCPRegistryPhaseSyncing
 	}
 
-	// If sync is complete or idle (no sync needed), check API status
-	if syncPhase == SyncPhaseComplete || syncPhase == SyncPhaseIdle {
+	// If sync is complete (no sync needed), check API status
+	if syncPhase == SyncPhaseComplete {
 		switch apiPhase {
 		case APIPhaseReady:
 			return MCPRegistryPhaseReady

cmd/thv-operator/api/v1alpha1/mcpregistry_types_test.go

@@ -131,63 +131,6 @@ func TestMCPRegistry_DeriveOverallPhase(t *testing.T) {
 			description:   "Sync complete + API unhealthy should result in Pending",
 		},
 
-		// Sync Idle + API combinations (key test cases for the recent fix)
-		{
-			name: "sync_idle_api_ready",
-			syncStatus: &SyncStatus{
-				Phase: SyncPhaseIdle,
-			},
-			apiStatus: &APIStatus{
-				Phase: APIPhaseReady,
-			},
-			expectedPhase: MCPRegistryPhaseReady,
-			description:   "Sync idle + API ready should result in Ready (fixed behavior)",
-		},
-		{
-			name: "sync_idle_api_error",
-			syncStatus: &SyncStatus{
-				Phase: SyncPhaseIdle,
-			},
-			apiStatus: &APIStatus{
-				Phase: APIPhaseError,
-			},
-			expectedPhase: MCPRegistryPhaseFailed,
-			description:   "Sync idle + API error should result in Failed",
-		},
-		{
-			name: "sync_idle_api_notstarted",
-			syncStatus: &SyncStatus{
-				Phase: SyncPhaseIdle,
-			},
-			apiStatus: &APIStatus{
-				Phase: APIPhaseNotStarted,
-			},
-			expectedPhase: MCPRegistryPhasePending,
-			description:   "Sync idle + API not started should result in Pending",
-		},
-		{
-			name: "sync_idle_api_deploying",
-			syncStatus: &SyncStatus{
-				Phase: SyncPhaseIdle,
-			},
-			apiStatus: &APIStatus{
-				Phase: APIPhaseDeploying,
-			},
-			expectedPhase: MCPRegistryPhasePending,
-			description:   "Sync idle + API deploying should result in Pending",
-		},
-		{
-			name: "sync_idle_api_unhealthy",
-			syncStatus: &SyncStatus{
-				Phase: SyncPhaseIdle,
-			},
-			apiStatus: &APIStatus{
-				Phase: APIPhaseUnhealthy,
-			},
-			expectedPhase: MCPRegistryPhasePending,
-			description:   "Sync idle + API unhealthy should result in Pending",
-		},
-
 		// Partial status combinations (one nil, one set)
 		{
 			name:          "sync_complete_api_nil",
@@ -200,19 +143,8 @@ func TestMCPRegistry_DeriveOverallPhase(t *testing.T) {
 			name:          "sync_nil_api_ready",
 			syncStatus:    nil,
 			apiStatus:     &APIStatus{Phase: APIPhaseReady},
-			expectedPhase: MCPRegistryPhaseReady,
-			description:   "Sync nil + API ready should result in Ready (sync defaults to Idle, which is treated as valid)",
-		},
-
-		// Edge case: sync idle with nil API (common in real scenarios)
-		{
-			name: "sync_idle_api_nil",
-			syncStatus: &SyncStatus{
-				Phase: SyncPhaseIdle,
-			},
-			apiStatus:     nil,
 			expectedPhase: MCPRegistryPhasePending,
-			description:   "Sync idle + API nil should result in Pending (API defaults to NotStarted)",
+			description:   "Sync nil + API ready should result in Pending",
 		},
 	}
 
@@ -247,7 +179,7 @@ func TestMCPRegistry_DeriveOverallPhase(t *testing.T) {
 // Helper function to get sync phase as string for better test output
 func getSyncPhaseString(syncStatus *SyncStatus) string {
 	if syncStatus == nil {
-		return "nil (defaults to Idle)"
+		return "nil"
 	}
 	return string(syncStatus.Phase)
 }
@@ -264,20 +196,20 @@ func getAPIPhaseString(apiStatus *APIStatus) string {
 func TestMCPRegistry_DeriveOverallPhase_EdgeCases(t *testing.T) {
 	t.Parallel()
 
-	t.Run("regression_test_idle_ready_becomes_ready", func(t *testing.T) {
+	t.Run("regression_test_complete_ready_becomes_ready", func(t *testing.T) {
 		t.Parallel()
 		// This is the specific regression test for the bug fix where
-		// syncPhase=Idle + apiPhase=Ready was incorrectly returning Pending
+		// syncPhase=Complete + apiPhase=Ready was incorrectly returning Pending
 		registry := &MCPRegistry{
 			Status: MCPRegistryStatus{
-				SyncStatus: &SyncStatus{Phase: SyncPhaseIdle},
+				SyncStatus: &SyncStatus{Phase: SyncPhaseComplete},
 				APIStatus:  &APIStatus{Phase: APIPhaseReady},
 			},
 		}
 
 		phase := registry.DeriveOverallPhase()
 		assert.Equal(t, MCPRegistryPhaseReady, phase,
-			"The specific case syncPhase=Idle + apiPhase=Ready should result in Ready phase")
+			"The specific case syncPhase=Complete + apiPhase=Ready should result in Ready phase")
 	})
 
 	t.Run("all_api_phases_with_failed_sync", func(t *testing.T) {