From 53df1bcc983fc9d19796415b445f42ea8fb43485 Mon Sep 17 00:00:00 2001 From: z1cheng Date: Fri, 1 Dec 2023 13:27:07 +0800 Subject: [PATCH] refactor: remove redundant the returned error Signed-off-by: z1cheng --- pkg/controllers/nsautoprop/controller.go | 8 +-- pkg/controllers/scheduler/scheduler.go | 6 +- .../scheduler/schedulingtriggers.go | 15 ++-- .../scheduler/schedulingtriggers_test.go | 6 +- pkg/controllers/status/controller.go | 5 +- .../statusaggregator/plugins/deployment.go | 10 +-- pkg/controllers/sync/resource.go | 4 +- pkg/util/annotation/annotation.go | 68 +++++-------------- pkg/util/annotation/annotation_test.go | 58 ++-------------- .../pendingcontrollers/pendingcontrollers.go | 5 +- 10 files changed, 37 insertions(+), 148 deletions(-) diff --git a/pkg/controllers/nsautoprop/controller.go b/pkg/controllers/nsautoprop/controller.go index 42709b70..003e8494 100644 --- a/pkg/controllers/nsautoprop/controller.go +++ b/pkg/controllers/nsautoprop/controller.go @@ -18,7 +18,6 @@ package nsautoprop import ( "context" - "fmt" "regexp" "strings" "time" @@ -371,12 +370,7 @@ func (c *Controller) ensureAnnotation( fedNamespace *fedcorev1a1.ClusterFederatedObject, key, value string, ) (bool, error) { - needsUpdate, err := annotationutil.AddAnnotation(fedNamespace, key, value) - if err != nil { - return false, fmt.Errorf( - "failed to add %s annotation to %s, err: %w", - key, fedNamespace.GetName(), err) - } + needsUpdate := annotationutil.AddAnnotation(fedNamespace, key, value) return needsUpdate, nil } diff --git a/pkg/controllers/scheduler/scheduler.go b/pkg/controllers/scheduler/scheduler.go index 02874b84..b77090f5 100644 --- a/pkg/controllers/scheduler/scheduler.go +++ b/pkg/controllers/scheduler/scheduler.go @@ -481,11 +481,7 @@ func (s *Scheduler) prepareToSchedule( logger.Error(err, "Failed to compute scheduling annotations") return nil, nil, nil, &worker.StatusError } - annotationChanged, err := updateSchedulingAnnotations(triggersText, deferredReasons, fedObject) - if err != nil { - logger.Error(err, "Failed to update scheduling annotations") - return nil, nil, nil, &worker.StatusError - } + annotationChanged := updateSchedulingAnnotations(triggersText, deferredReasons, fedObject) shouldSkipScheduling := false if !triggersChanged { diff --git a/pkg/controllers/scheduler/schedulingtriggers.go b/pkg/controllers/scheduler/schedulingtriggers.go index e7166940..95a528c2 100644 --- a/pkg/controllers/scheduler/schedulingtriggers.go +++ b/pkg/controllers/scheduler/schedulingtriggers.go @@ -225,17 +225,14 @@ func computeSchedulingAnnotations( func updateSchedulingAnnotations( triggers, deferredReasons string, fedObject fedcorev1a1.GenericFederatedObject, -) (annotationChanged bool, err error) { - triggersChanged, err := annotation.AddAnnotation(fedObject, SchedulingTriggersAnnotation, triggers) - if err != nil { - return false, err - } +) (annotationChanged bool) { + triggersChanged := annotation.AddAnnotation(fedObject, SchedulingTriggersAnnotation, triggers) if len(deferredReasons) == 0 { - deferred, err := annotation.RemoveAnnotation(fedObject, SchedulingDeferredReasonsAnnotation) - return triggersChanged || deferred, err + deferred := annotation.RemoveAnnotation(fedObject, SchedulingDeferredReasonsAnnotation) + return triggersChanged || deferred } - deferred, err := annotation.AddAnnotation(fedObject, SchedulingDeferredReasonsAnnotation, deferredReasons) - return triggersChanged || deferred, err + deferred := annotation.AddAnnotation(fedObject, SchedulingDeferredReasonsAnnotation, deferredReasons) + return triggersChanged || deferred } func computeSchedulingTriggers( diff --git a/pkg/controllers/scheduler/schedulingtriggers_test.go b/pkg/controllers/scheduler/schedulingtriggers_test.go index 6b5bed40..ff81ef39 100644 --- a/pkg/controllers/scheduler/schedulingtriggers_test.go +++ b/pkg/controllers/scheduler/schedulingtriggers_test.go @@ -614,11 +614,7 @@ func Test_computeSchedulingAnnotations(t *testing.T) { t.Errorf("Marshal() unexpected err: %v", err) return } - _, err = annotation.AddAnnotation(fedObj, SchedulingTriggersAnnotation, oldTriggerText) - if err != nil { - t.Errorf("AddAnnotation() unexpected err: %v", err) - return - } + annotation.AddAnnotation(fedObj, SchedulingTriggersAnnotation, oldTriggerText) } newPolicy := generatePolicy(tt.newStatus.policyName, tt.newStatus.reschedulePolicy) diff --git a/pkg/controllers/status/controller.go b/pkg/controllers/status/controller.go index 24f6d8bb..0b2cbef3 100644 --- a/pkg/controllers/status/controller.go +++ b/pkg/controllers/status/controller.go @@ -414,14 +414,11 @@ func (s *StatusController) reconcile( var hasRSDigestsAnnotation bool if existingStatus != nil { - hasRSDigestsAnnotation, err = annotation.HasAnnotationKeyValue( + hasRSDigestsAnnotation = annotation.HasAnnotationKeyValue( existingStatus, common.LatestReplicasetDigestsAnnotation, rsDigestsAnnotation, ) - if err != nil { - return worker.StatusError - } } collectedStatus := newCollectedStatusObject(fedObject, clusterStatuses) diff --git a/pkg/controllers/statusaggregator/plugins/deployment.go b/pkg/controllers/statusaggregator/plugins/deployment.go index 0a707fa2..a6af31cd 100644 --- a/pkg/controllers/statusaggregator/plugins/deployment.go +++ b/pkg/controllers/statusaggregator/plugins/deployment.go @@ -156,14 +156,11 @@ func (receiver *DeploymentPlugin) AggregateStatuses( } rsDigestsAnnotation := string(rsDigestsAnnotationBytes) - hasRSDigestsAnnotation, err := annotation.HasAnnotationKeyValue( + hasRSDigestsAnnotation := annotation.HasAnnotationKeyValue( sourceObject, common.LatestReplicasetDigestsAnnotation, rsDigestsAnnotation, ) - if err != nil { - return nil, false, err - } if hasRSDigestsAnnotation { return sourceObject, needUpdate, nil @@ -171,10 +168,7 @@ func (receiver *DeploymentPlugin) AggregateStatuses( needUpdate = true } - _, err = annotation.AddAnnotation(sourceObject, common.LatestReplicasetDigestsAnnotation, rsDigestsAnnotation) - if err != nil { - return nil, false, err - } + annotation.AddAnnotation(sourceObject, common.LatestReplicasetDigestsAnnotation, rsDigestsAnnotation) return sourceObject, needUpdate, nil } diff --git a/pkg/controllers/sync/resource.go b/pkg/controllers/sync/resource.go index 07c1f0d1..30ebade1 100644 --- a/pkg/controllers/sync/resource.go +++ b/pkg/controllers/sync/resource.go @@ -166,9 +166,7 @@ func (r *federatedResource) ObjectForCluster(clusterName string) (*unstructured. obj := r.template.DeepCopy() if obj.GetGeneration() != 0 { - if _, err := annotationutil.AddAnnotation(obj, common.SourceGenerationAnnotation, fmt.Sprintf("%d", obj.GetGeneration())); err != nil { - return nil, err - } + annotationutil.AddAnnotation(obj, common.SourceGenerationAnnotation, fmt.Sprintf("%d", obj.GetGeneration())) } switch r.TargetGVK() { diff --git a/pkg/util/annotation/annotation.go b/pkg/util/annotation/annotation.go index 108b7d60..d3ee6cfa 100644 --- a/pkg/util/annotation/annotation.go +++ b/pkg/util/annotation/annotation.go @@ -17,9 +17,6 @@ limitations under the License. package annotation import ( - "fmt" - "reflect" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -30,49 +27,32 @@ const ( ) // HasAnnotationKey returns true if the given object has the given annotation key in its ObjectMeta. -func HasAnnotationKey(obj metav1.Object, key string) (bool, error) { - if IsNilPointer(obj) { - return false, fmt.Errorf("object(%T) is nil pointer", obj) - } +func HasAnnotationKey(obj metav1.Object, key string) bool { annotations := obj.GetAnnotations() if annotations == nil { - return false, nil + return false } _, ok := annotations[key] - return ok, nil + return ok } // HasAnnotationKeyValue returns true if the given object has the given annotation key and value in its ObjectMeta. -func HasAnnotationKeyValue(obj metav1.Object, key, value string) (bool, error) { - if IsNilPointer(obj) { - return false, fmt.Errorf("object(%T) is nil pointer", obj) - } +func HasAnnotationKeyValue(obj metav1.Object, key, value string) bool { annotations := obj.GetAnnotations() if annotations == nil { - return false, nil + return false } val, ok := annotations[key] - return ok && value == val, nil + return ok && value == val } // AddAnnotation adds the given annotation key and value to the given objects ObjectMeta, // and overwrites the annotation value if it already exists. // Returns true if the object was updated. -func AddAnnotation(obj metav1.Object, key, value string) (bool, error) { - if IsNilPointer(obj) { - return false, fmt.Errorf("object(%T) is nil pointer", obj) - } - - if key == "" { - return false, fmt.Errorf("key is a empty string.") - } - - has, err := HasAnnotationKeyValue(obj, key, value) - if has && err == nil { - return false, nil - } - if err != nil { - return false, err +func AddAnnotation(obj metav1.Object, key, value string) bool { + has := HasAnnotationKeyValue(obj, key, value) + if has { + return false } annotations := obj.GetAnnotations() if annotations == nil { @@ -80,37 +60,23 @@ func AddAnnotation(obj metav1.Object, key, value string) (bool, error) { } annotations[key] = value obj.SetAnnotations(annotations) - return true, nil + return true } // RemoveAnnotation removes the given annotation key from the given objects ObjectMeta. // Returns true if the object was updated. -func RemoveAnnotation(obj metav1.Object, key string) (bool, error) { - if IsNilPointer(obj) { - return false, fmt.Errorf("object(%T) is nil pointer", obj) - } - has, err := HasAnnotationKey(obj, key) - if !has && err == nil { - return false, nil - } - if err != nil { - return false, err +func RemoveAnnotation(obj metav1.Object, key string) bool { + has := HasAnnotationKey(obj, key) + if !has { + return false } annotations := obj.GetAnnotations() if annotations == nil { - return false, nil + return false } delete(annotations, key) obj.SetAnnotations(annotations) - return true, nil -} - -// IsNilPointer returns true if i is nil pointer or value of i is nil. -func IsNilPointer(i interface{}) bool { - if i == nil || (reflect.ValueOf(i).Kind() == reflect.Ptr && reflect.ValueOf(i).IsNil()) { - return true - } - return false + return true } diff --git a/pkg/util/annotation/annotation_test.go b/pkg/util/annotation/annotation_test.go index 7e8c59d4..b2fcaf70 100644 --- a/pkg/util/annotation/annotation_test.go +++ b/pkg/util/annotation/annotation_test.go @@ -22,7 +22,6 @@ import ( "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" - meta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -38,11 +37,6 @@ func TestHasAnnotationKey(t *testing.T) { annotation string result bool }{ - { - newObj(map[string]string{}), - "", - false, - }, { newObj(map[string]string{}), "someAnnotation", @@ -70,7 +64,7 @@ func TestHasAnnotationKey(t *testing.T) { }, } for index, test := range testCases { - hasAnnotationKey, _ := HasAnnotationKey(test.obj, test.annotation) + hasAnnotationKey := HasAnnotationKey(test.obj, test.annotation) assert.Equal( t, hasAnnotationKey, @@ -87,24 +81,12 @@ func TestHasAnnotationKeyValue(t *testing.T) { value string result bool }{ - { - newObj(map[string]string{}), - "", - "", - false, - }, { newObj(map[string]string{}), "someAnnotation", "", false, }, - { - newObj(map[string]string{"someAnnotation": ""}), - "", - "", - false, - }, { newObj(map[string]string{"someAnnotation": ""}), "anotherAnnotation", @@ -137,7 +119,7 @@ func TestHasAnnotationKeyValue(t *testing.T) { }, } for index, test := range testCases { - hasAnnotationKeyValue, _ := HasAnnotationKeyValue(test.obj, test.key, test.value) + hasAnnotationKeyValue := HasAnnotationKeyValue(test.obj, test.key, test.value) assert.Equal( t, hasAnnotationKeyValue, @@ -155,20 +137,6 @@ func TestAddAnnotation(t *testing.T) { isUpdated bool newAnnotations map[string]string }{ - { - newObj(map[string]string{}), - "", - "", - false, - map[string]string{}, - }, - { - newObj(nil), - "", - "", - false, - map[string]string(nil), - }, { newObj(map[string]string{}), "someAnnotation", @@ -206,15 +174,14 @@ func TestAddAnnotation(t *testing.T) { }, } for index, test := range testCases { - isUpdated, _ := AddAnnotation(test.obj, test.key, test.value) + isUpdated := AddAnnotation(test.obj, test.key, test.value) assert.Equal( t, isUpdated, test.isUpdated, fmt.Sprintf("Test case %d failed. Expected isUpdated: %v, actual: %v", index, test.isUpdated, isUpdated), ) - accessor, _ := meta.Accessor(test.obj) - newAnnotations := accessor.GetAnnotations() + newAnnotations := test.obj.GetAnnotations() assert.Equal( t, test.newAnnotations, @@ -236,18 +203,6 @@ func TestRemoveAnnotation(t *testing.T) { isUpdated bool newAnnotations map[string]string }{ - { - newObj(map[string]string{}), - "", - false, - map[string]string{}, - }, - { - newObj(nil), - "", - false, - nil, - }, { newObj(map[string]string{}), "someAnnotation", @@ -268,15 +223,14 @@ func TestRemoveAnnotation(t *testing.T) { }, } for index, test := range testCases { - isUpdated, _ := RemoveAnnotation(test.obj, test.key) + isUpdated := RemoveAnnotation(test.obj, test.key) assert.Equal( t, isUpdated, test.isUpdated, fmt.Sprintf("Test case %d failed. Expected isUpdated: %v, actual: %v", index, test.isUpdated, isUpdated), ) - accessor, _ := meta.Accessor(test.obj) - newAnnotations := accessor.GetAnnotations() + newAnnotations := test.obj.GetAnnotations() assert.Equal( t, test.newAnnotations, diff --git a/pkg/util/pendingcontrollers/pendingcontrollers.go b/pkg/util/pendingcontrollers/pendingcontrollers.go index def421b1..64e77f2f 100644 --- a/pkg/util/pendingcontrollers/pendingcontrollers.go +++ b/pkg/util/pendingcontrollers/pendingcontrollers.go @@ -71,10 +71,7 @@ func SetPendingControllers( if err != nil { return false, fmt.Errorf("failed to marshal json: %w", err) } - updated, err = annotationutil.AddAnnotation(fedObject, PendingControllersAnnotation, string(annotationValue)) - if err != nil { - return updated, fmt.Errorf("failed to add annotation: %w", err) - } + updated = annotationutil.AddAnnotation(fedObject, PendingControllersAnnotation, string(annotationValue)) return updated, err }