Revert "address comments to fix bug (Azure#1080)"

This reverts commit 9a376db.

Author: zhiying-lin

pkg/controllers/workapplier/availability_tracker.go

@@ -271,11 +271,11 @@ func trackServiceExportAvailability(curObj *unstructured.Unstructured) (Manifest
 		return ManifestProcessingAvailabilityResultTypeNotYetAvailable, nil
 	}
 	// Validate annotation weight. Updating the annotation won't change the object generation,
-	// so the current status is not reliable and need to validate the annotation again here.
+	// so the current status is not reliable and need to validate the annotation again here
 	weight, err := objectmeta.ExtractWeightFromServiceExport(&svcExport)
 	if err != nil {
-		klog.Error(err, "ServiceExport has invalid weight", "serviceExport", svcExportObj)
-		return ManifestProcessingAvailabilityResultTypeNotYetAvailable, nil
+		klog.Errorf(err.Error(), "ServiceExport has invalid weight", "serviceExport", svcExportObj)
+		return ManifestProcessingAvailabilityResultTypeNotYetAvailable, err
 	}
 	if weight != 0 {
 		// Check conflict condition for non-zero weight

pkg/controllers/workapplier/availability_tracker_test.go

@@ -7,6 +7,7 @@ package workapplier
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -1001,6 +1002,7 @@ func TestServiceExportAvailability(t *testing.T) {
 		weight                                       string
 		status                                       fleetnetworkingv1alpha1.ServiceExportStatus
 		wantManifestProcessingAvailabilityResultType ManifestProcessingAvailabilityResultType
+		err                                          error
 	}{
 		{
 			name:   "available svcExport (annotation weight is 0)",
@@ -1016,6 +1018,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport (ServiceExportValid is false)",
@@ -1031,6 +1034,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport (different generation, annotation weight is 0)",
@@ -1046,6 +1050,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "available svcExport with no conflict (annotation weight is 1)",
@@ -1067,6 +1072,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport with conflict (annotation weight is 1)",
@@ -1088,6 +1094,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable invalid svcExport (annotation weight is 1)",
@@ -1103,6 +1110,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport (different generation, annotation weight is 1)",
@@ -1124,6 +1132,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport (no annotation weight, no conflict condition)",
@@ -1139,6 +1148,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "available svcExport (no annotation weight)",
@@ -1160,6 +1170,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport (no annotation weight with conflict)",
@@ -1181,6 +1192,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport (no annotation weight, different generation)",
@@ -1202,6 +1214,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport (no annotation weight, invalid service export)",
@@ -1217,6 +1230,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: nil,
 		},
 		{
 			name:   "unavailable svcExport (invalid weight)",
@@ -1232,6 +1246,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: fmt.Errorf("the weight annotation is not a valid integer: a"),
 		},
 		{
 			name:   "unavailable svcExport (out of range weight)",
@@ -1247,6 +1262,7 @@ func TestServiceExportAvailability(t *testing.T) {
 				},
 			},
 			wantManifestProcessingAvailabilityResultType: ManifestProcessingAvailabilityResultTypeNotYetAvailable,
+			err: fmt.Errorf("the weight annotation is not in the range [0, 1000]: 1002"),
 		},
 	}
 
@@ -1258,8 +1274,14 @@ func TestServiceExportAvailability(t *testing.T) {
 			}
 			svcExport.Status = tc.status
 			gotResTyp, err := trackServiceExportAvailability(toUnstructured(t, svcExport))
+
+			// Check for errors
 			if err != nil {
-				t.Fatalf("trackServiceExportAvailability() = %v, want no error", err)
+				if tc.err == nil || err.Error() != tc.err.Error() {
+					t.Fatalf("trackServiceExportAvailability() = %v, want %v", err, tc.err)
+				}
+			} else if tc.err != nil {
+				t.Fatalf("trackServiceExportAvailability() = nil, want error: %v", tc.err)
 			}
 
 			// Check the result type