feat: unified RecordStatuses and RecordErrorConditions (#56)

* feat: unified RecordStatuses and RecordErrorConditions

* remove useless test println

Author: WeichengWang1

README.md

@@ -3,8 +3,8 @@
 customized controller can be realized easily, and offering the ability of following 
 [**PodOpsLifecycle**](https://kusionstack.io/docs/operating/concepts/podopslifecycle) for controllers.
 
-The only thing users need to do to realize a customized controller is writing an adapter implementing 
-[ReconcileAdapter](https://github.com/KusionStack/resourceconsist/blob/main/pkg/frame/controller/types.go#L93). 
+The only thing users need to do to realize a customized controller is writing an adapter implementing
+[ReconcileAdapter](https://github.com/KusionStack/resourceconsist/blob/main/pkg/frame/controller/types.go#L100). 
 For controllers following **PodOpsLifecycle**, 
 [WebhookAdapter](https://github.com/KusionStack/resourceconsist/blob/main/pkg/frame/webhook/types.go#L26) is 
 also necessary to be implemented.
@@ -30,7 +30,7 @@ is an adapter handles resources by itself, while [SlbControllerAdapter](https://
 is an adapter handles resources cooperate with CCM controller.
 
 With **ResourceConsist**, you can build a bridge between existing controllers and PodOpsLifecycle, just like what
-SlbControllerAdapter](https://github.com/KusionStack/resourceconsist/tree/main/pkg/adapters/alibabacloudslb/alibabacloudslb_controller.go#L35) did.
+[SlbControllerAdapter](https://github.com/KusionStack/resourceconsist/tree/main/pkg/adapters/alibabacloudslb/alibabacloudslb_controller.go#L35) did.
 
 Please visit [tutorial](https://github.com/KusionStack/resourceconsist/tree/main/docs/tutorial.md) to start a controller.
 ## ☎️ Contact us

docs/keyconcepts.md

@@ -85,8 +85,11 @@ type ExpectedFinalizerRecordOptions interface {
 
 // StatusRecordOptions defines methods of record something for adapters
 type StatusRecordOptions interface {
+    // RecordStatuses records statuses of employer and employees, called at the end of reconcile.
+    // cudEmployerResults and cudEmployeeResults are the results of create/update/delete operations on employer and employees
+    // err indicates if reconcile failed, if err is not nil, error condition will be recorded, otherwise status will be recorded
     RecordStatuses(ctx context.Context, employer client.Object,
-    cudEmployerResults CUDEmployerResults, cudEmployeeResults CUDEmployeeResults) error
+    cudEmployerResults CUDEmployerResults, cudEmployeeResults CUDEmployeeResults, err error) error
 }
 
 // ReconcileLifecycleOptions defines whether PodOpsLifecycle followed

docs/tutorial.md

@@ -3,8 +3,8 @@
 
 The frame, ```kusionstack.io/resourceconsist/pkg/frame```, is used for adapters starting a controller, which handles 
 Reconcile and Employer/Employees' spec&status. If you wrote an adapter in your own repo, you can import 
-```kusionstack.io/resourceconsist/pkg/frame/controller``` and ```kusionstack.io/resourceconsist/pkg/frame/webhook```, 
-]and call AddToMgr to start a controller.
+```kusionstack.io/resourceconsist/pkg/frame/controller``` and ```kusionstack.io/resourceconsist/pkg/frame/webhook```,
+and call AddToMgr to start a controller.
 
 >webhookAdapter is only necessary to be implemented for controllers following PodOpsLifecycle.
 ```Go
@@ -93,7 +93,7 @@ type DemoControllerAdapter struct {
 }
 ```
 Declaring that the DemoControllerAdapter implemented ```ReconcileAdapter``` and ```ReconcileLifecycleOptions```. 
-Implementing ```RconcileAdapter``` is a must action, while ```ReconcileLifecycleOptions``` isn't, check the remarks 
+Implementing ```ReconcileAdapter``` is a must action, while ```ReconcileLifecycleOptions``` isn't, check the remarks 
 for ```ReconcileLifecycleOptions``` in ```kusionstack.io/resourceconsist/pkg/frame/controller/types.go``` to find why.
 ```Go
 var _ ReconcileAdapter = &DemoControllerAdapter{}
@@ -116,13 +116,15 @@ employee.
 type IEmployer interface {
 	GetEmployerId() string
 	GetEmployerStatuses() interface{}
+	SetEmployerStatuses(employerStatuses interface{})
 	EmployerEqual(employer IEmployer) (bool, error)
 }
 
 type IEmployee interface {
 	GetEmployeeId() string
 	GetEmployeeName() string
 	GetEmployeeStatuses() interface{}
+	SetEmployeeStatuses(employeeStatuses interface{})
 	EmployeeEqual(employee IEmployee) (bool, error)
 }
 
@@ -208,13 +210,13 @@ related backend provider. Here in the demo adapter, ```CreateEmployer/UpdateEmpl
 ```demoResourceVipStatusInProvider```.
 ```Go
 func (r *DemoControllerAdapter) CreateEmployer(ctx context.Context, employer client.Object, toCreates []IEmployer) ([]IEmployer, []IEmployer, error) {
-	if toCreates == nil || len(toCreates) == 0 {
+	if len(toCreates) == 0 {
 		return toCreates, nil, nil
 	}
 
-	toCreateDemoServiceStatus := make([]DemoServiceStatus, len(toCreates))
+	toCreateDemoServiceStatus := make([]*DemoServiceStatus, len(toCreates))
 	for idx, create := range toCreates {
-		createDemoServiceStatus, ok := create.(DemoServiceStatus)
+		createDemoServiceStatus, ok := create.(*DemoServiceStatus)
 		if !ok {
 			return nil, toCreates, fmt.Errorf("toCreates employer is not DemoServiceStatus")
 		}
@@ -231,13 +233,13 @@ func (r *DemoControllerAdapter) CreateEmployer(ctx context.Context, employer cli
 }
 
 func (r *DemoControllerAdapter) UpdateEmployer(ctx context.Context, employer client.Object, toUpdates []IEmployer) ([]IEmployer, []IEmployer, error) {
-	if toUpdates == nil || len(toUpdates) == 0 {
+	if len(toUpdates) == 0 {
 		return toUpdates, nil, nil
 	}
 
-	toUpdateDemoServiceStatus := make([]DemoServiceStatus, len(toUpdates))
+	toUpdateDemoServiceStatus := make([]*DemoServiceStatus, len(toUpdates))
 	for idx, update := range toUpdates {
-		updateDemoServiceStatus, ok := update.(DemoServiceStatus)
+		updateDemoServiceStatus, ok := update.(*DemoServiceStatus)
 		if !ok {
 			return nil, toUpdates, fmt.Errorf("toUpdates employer is not DemoServiceStatus")
 		}
@@ -254,13 +256,13 @@ func (r *DemoControllerAdapter) UpdateEmployer(ctx context.Context, employer cli
 }
 
 func (r *DemoControllerAdapter) DeleteEmployer(ctx context.Context, employer client.Object, toDeletes []IEmployer) ([]IEmployer, []IEmployer, error) {
-	if toDeletes == nil || len(toDeletes) == 0 {
+	if len(toDeletes) == 0 {
 		return toDeletes, nil, nil
 	}
 
-	toDeleteDemoServiceStatus := make([]DemoServiceStatus, len(toDeletes))
+	toDeleteDemoServiceStatus := make([]*DemoServiceStatus, len(toDeletes))
 	for idx, update := range toDeletes {
-		deleteDemoServiceStatus, ok := update.(DemoServiceStatus)
+		deleteDemoServiceStatus, ok := update.(*DemoServiceStatus)
 		if !ok {
 			return nil, toDeletes, fmt.Errorf("toDeletes employer is not DemoServiceStatus")
 		}
@@ -351,13 +353,13 @@ on related backend provider. Here in the demo adapter, ```CreateEmployees/Update
 handles ```demoResourceRsStatusInProvider```.
 ```Go
 func (r *DemoControllerAdapter) CreateEmployees(ctx context.Context, employer client.Object, toCreates []IEmployee) ([]IEmployee, []IEmployee, error) {
-	if toCreates == nil || len(toCreates) == 0 {
+	if len(toCreates) == 0 {
 		return toCreates, nil, nil
 	}
-	toCreateDemoPodStatuses := make([]DemoPodStatus, len(toCreates))
+	toCreateDemoPodStatuses := make([]*DemoPodStatus, len(toCreates))
 
 	for idx, toCreate := range toCreates {
-		podStatus, ok := toCreate.(DemoPodStatus)
+		podStatus, ok := toCreate.(*DemoPodStatus)
 		if !ok {
 			return nil, toCreates, fmt.Errorf("toCreate is not DemoPodStatus")
 		}
@@ -375,14 +377,14 @@ func (r *DemoControllerAdapter) CreateEmployees(ctx context.Context, employer cl
 }
 
 func (r *DemoControllerAdapter) UpdateEmployees(ctx context.Context, employer client.Object, toUpdates []IEmployee) ([]IEmployee, []IEmployee, error) {
-	if toUpdates == nil || len(toUpdates) == 0 {
+	if len(toUpdates) == 0 {
 		return toUpdates, nil, nil
 	}
 
-	toUpdateDemoPodStatuses := make([]DemoPodStatus, len(toUpdates))
+	toUpdateDemoPodStatuses := make([]*DemoPodStatus, len(toUpdates))
 
 	for idx, toUpdate := range toUpdates {
-		podStatus, ok := toUpdate.(DemoPodStatus)
+		podStatus, ok := toUpdate.(*DemoPodStatus)
 		if !ok {
 			return nil, toUpdates, fmt.Errorf("toUpdate is not DemoPodStatus")
 		}
@@ -400,14 +402,14 @@ func (r *DemoControllerAdapter) UpdateEmployees(ctx context.Context, employer cl
 }
 
 func (r *DemoControllerAdapter) DeleteEmployees(ctx context.Context, employer client.Object, toDeletes []IEmployee) ([]IEmployee, []IEmployee, error) {
-	if toDeletes == nil || len(toDeletes) == 0 {
+	if len(toDeletes) == 0 {
 		return toDeletes, nil, nil
 	}
 
-	toDeleteDemoPodStatuses := make([]DemoPodStatus, len(toDeletes))
+	toDeleteDemoPodStatuses := make([]*DemoPodStatus, len(toDeletes))
 
 	for idx, toDelete := range toDeletes {
-		podStatus, ok := toDelete.(DemoPodStatus)
+		podStatus, ok := toDelete.(*DemoPodStatus)
 		if !ok {
 			return nil, toDeletes, fmt.Errorf("toDelete is not DemoPodStatus")
 		}

pkg/frame/controller/resourceconsist_controller.go

@@ -162,7 +162,7 @@ func (r *Consist) Reconcile(ctx context.Context, request reconcile.Request) (rec
 	defer func() {
 		if err != nil {
 			if recordOptions, ok := r.adapter.(StatusRecordOptions); ok {
-				err = recordOptions.RecordErrorConditions(ctx, employer, err)
+				err = recordOptions.RecordStatuses(ctx, employer, CUDEmployerResults{}, CUDEmployeeResults{}, err)
 				if err != nil {
 					logger.Error(err, "record error conditions failed")
 					r.recorder.Eventf(employer, corev1.EventTypeWarning, RecordErrorConditionsFailed,
@@ -255,7 +255,7 @@ func (r *Consist) Reconcile(ctx context.Context, request reconcile.Request) (rec
 	}
 
 	if recordOptions, ok := r.adapter.(StatusRecordOptions); ok {
-		err = recordOptions.RecordStatuses(ctx, employer, cudEmployerResults, cudEmployeeResults)
+		err = recordOptions.RecordStatuses(ctx, employer, cudEmployerResults, cudEmployeeResults, err)
 		if err != nil {
 			logger.Error(err, "record status failed")
 			r.recorder.Eventf(employer, corev1.EventTypeWarning, RecordStatusesFailed,

pkg/frame/controller/resourceconsist_controller_suite_test.go

@@ -47,20 +47,24 @@ func NewDemoReconcileAdapter(c client.Client, rc *DemoResourceProviderClient) Re
 }
 
 func (r *DemoControllerAdapter) RecordStatuses(ctx context.Context, employer client.Object,
-	cudEmployerResults CUDEmployerResults, cudEmployeeResults CUDEmployeeResults) error {
-	if employer.GetAnnotations()["resource-consist.kusionstack.io/demo-condition"] != "" {
-		employer.GetAnnotations()["resource-consist.kusionstack.io/demo-condition"] = ""
-		return r.Update(ctx, employer)
-	}
-	return nil
-}
-
-func (r *DemoControllerAdapter) RecordErrorConditions(ctx context.Context, employer client.Object, err error) error {
+	cudEmployerResults CUDEmployerResults, cudEmployeeResults CUDEmployeeResults, err error) error {
 	annos := employer.GetAnnotations()
 	if annos == nil {
 		annos = make(map[string]string)
 	}
-	annos["resource-consist.kusionstack.io/demo-condition"] = err.Error()
+
+	if err != nil {
+		// Record error condition
+		annos["resource-consist.kusionstack.io/demo-condition"] = err.Error()
+	} else {
+		// Clear error condition if present
+		if annos["resource-consist.kusionstack.io/demo-condition"] != "" {
+			annos["resource-consist.kusionstack.io/demo-condition"] = ""
+		} else {
+			return nil
+		}
+	}
+
 	employer.SetAnnotations(annos)
 	return r.Update(ctx, employer)
 }

pkg/frame/controller/types.go

@@ -68,14 +68,11 @@ type ExpectedFinalizerRecordOptions interface {
 
 // StatusRecordOptions defines methods of record something for adapters
 type StatusRecordOptions interface {
-	// RecordStatuses records statuses of employer and employees, called at the end of successful reconcile.
+	// RecordStatuses records statuses of employer and employees, called at the end of reconcile.
 	// cudEmployerResults and cudEmployeeResults are the results of create/update/delete operations on employer and employees
+	// err indicates if reconcile failed, if err is not nil, error condition will be recorded, otherwise status will be recorded
 	RecordStatuses(ctx context.Context, employer client.Object,
-		cudEmployerResults CUDEmployerResults, cudEmployeeResults CUDEmployeeResults) error
-
-	// RecordErrorConditions records error conditions, called at the end of failed reconcile.
-	// usually, something recorded in RecordErrorConditions should be erased in RecordStatuses.
-	RecordErrorConditions(ctx context.Context, employer client.Object, err error) error
+		cudEmployerResults CUDEmployerResults, cudEmployeeResults CUDEmployeeResults, err error) error
 }
 
 // ReconcileLifecycleOptions defines whether PodOpsLifecycle followed