Skip to content

Commit

Permalink
refactor: remove redundant the returned error
Browse files Browse the repository at this point in the history
Signed-off-by: z1cheng <[email protected]>
  • Loading branch information
z1cheng committed Dec 12, 2023
1 parent 6d35318 commit 019dafe
Show file tree
Hide file tree
Showing 10 changed files with 42 additions and 87 deletions.
8 changes: 1 addition & 7 deletions pkg/controllers/nsautoprop/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package nsautoprop

import (
"context"
"fmt"
"regexp"
"strings"
"time"
Expand Down Expand Up @@ -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)

Check warning on line 373 in pkg/controllers/nsautoprop/controller.go

View check run for this annotation

Codecov / codecov/patch

pkg/controllers/nsautoprop/controller.go#L373

Added line #L373 was not covered by tests

return needsUpdate, nil
}
Expand Down
6 changes: 1 addition & 5 deletions pkg/controllers/scheduler/scheduler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
15 changes: 6 additions & 9 deletions pkg/controllers/scheduler/schedulingtriggers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
6 changes: 1 addition & 5 deletions pkg/controllers/scheduler/schedulingtriggers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 1 addition & 4 deletions pkg/controllers/status/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,14 +414,11 @@ func (s *StatusController) reconcile(

var hasRSDigestsAnnotation bool
if existingStatus != nil {
hasRSDigestsAnnotation, err = annotation.HasAnnotationKeyValue(
hasRSDigestsAnnotation = annotation.HasAnnotationKeyValue(

Check warning on line 417 in pkg/controllers/status/controller.go

View check run for this annotation

Codecov / codecov/patch

pkg/controllers/status/controller.go#L417

Added line #L417 was not covered by tests
existingStatus,
common.LatestReplicasetDigestsAnnotation,
rsDigestsAnnotation,
)
if err != nil {
return worker.StatusError
}
}

collectedStatus := newCollectedStatusObject(fedObject, clusterStatuses)
Expand Down
10 changes: 2 additions & 8 deletions pkg/controllers/statusaggregator/plugins/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,25 +156,19 @@ func (receiver *DeploymentPlugin) AggregateStatuses(
}

rsDigestsAnnotation := string(rsDigestsAnnotationBytes)
hasRSDigestsAnnotation, err := annotation.HasAnnotationKeyValue(
hasRSDigestsAnnotation := annotation.HasAnnotationKeyValue(

Check warning on line 159 in pkg/controllers/statusaggregator/plugins/deployment.go

View check run for this annotation

Codecov / codecov/patch

pkg/controllers/statusaggregator/plugins/deployment.go#L159

Added line #L159 was not covered by tests
sourceObject,
common.LatestReplicasetDigestsAnnotation,
rsDigestsAnnotation,
)
if err != nil {
return nil, false, err
}

if hasRSDigestsAnnotation {
return sourceObject, needUpdate, nil
} else {
needUpdate = true
}

_, err = annotation.AddAnnotation(sourceObject, common.LatestReplicasetDigestsAnnotation, rsDigestsAnnotation)
if err != nil {
return nil, false, err
}
annotation.AddAnnotation(sourceObject, common.LatestReplicasetDigestsAnnotation, rsDigestsAnnotation)

Check warning on line 171 in pkg/controllers/statusaggregator/plugins/deployment.go

View check run for this annotation

Codecov / codecov/patch

pkg/controllers/statusaggregator/plugins/deployment.go#L171

Added line #L171 was not covered by tests

return sourceObject, needUpdate, nil
}
4 changes: 1 addition & 3 deletions pkg/controllers/sync/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()))

Check warning on line 169 in pkg/controllers/sync/resource.go

View check run for this annotation

Codecov / codecov/patch

pkg/controllers/sync/resource.go#L169

Added line #L169 was not covered by tests
}

switch r.TargetGVK() {
Expand Down
55 changes: 22 additions & 33 deletions pkg/util/annotation/annotation.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package annotation

import (
"fmt"
"reflect"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -30,81 +29,71 @@ 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) {
func HasAnnotationKey(obj metav1.Object, key string) bool {
if IsNilPointer(obj) {
return false, fmt.Errorf("object(%T) is nil pointer", obj)
return false

Check warning on line 34 in pkg/util/annotation/annotation.go

View check run for this annotation

Codecov / codecov/patch

pkg/util/annotation/annotation.go#L34

Added line #L34 was not covered by tests
}
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) {
func HasAnnotationKeyValue(obj metav1.Object, key, value string) bool {
if IsNilPointer(obj) {
return false, fmt.Errorf("object(%T) is nil pointer", obj)
return false

Check warning on line 47 in pkg/util/annotation/annotation.go

View check run for this annotation

Codecov / codecov/patch

pkg/util/annotation/annotation.go#L47

Added line #L47 was not covered by tests
}
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.")
func AddAnnotation(obj metav1.Object, key, value string) bool {
if IsNilPointer(obj) || key == "" {
return false
}

has, err := HasAnnotationKeyValue(obj, key, value)
if has && err == nil {
return false, nil
}
if err != nil {
return false, err
has := HasAnnotationKeyValue(obj, key, value)
if has {
return false
}
annotations := obj.GetAnnotations()
if annotations == nil {
annotations = make(map[string]string)
}
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) {
func RemoveAnnotation(obj metav1.Object, key string) bool {
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
return false

Check warning on line 82 in pkg/util/annotation/annotation.go

View check run for this annotation

Codecov / codecov/patch

pkg/util/annotation/annotation.go#L82

Added line #L82 was not covered by tests
}
if err != nil {
return false, err
has := HasAnnotationKey(obj, key)
if !has {
return false
}

annotations := obj.GetAnnotations()
if annotations == nil {
return false, nil
return false

Check warning on line 91 in pkg/util/annotation/annotation.go

View check run for this annotation

Codecov / codecov/patch

pkg/util/annotation/annotation.go#L91

Added line #L91 was not covered by tests
}

delete(annotations, key)
obj.SetAnnotations(annotations)
return true, nil
return true
}

// IsNilPointer returns true if i is nil pointer or value of i is nil.
Expand Down
15 changes: 6 additions & 9 deletions pkg/util/annotation/annotation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -70,7 +69,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,
Expand Down Expand Up @@ -137,7 +136,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,
Expand Down Expand Up @@ -206,15 +205,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,
Expand Down Expand Up @@ -268,15 +266,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,
Expand Down
5 changes: 1 addition & 4 deletions pkg/util/pendingcontrollers/pendingcontrollers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down

0 comments on commit 019dafe

Please sign in to comment.