Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove redundant returned error #290

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

z1cheng
Copy link
Contributor

@z1cheng z1cheng commented Dec 1, 2023

close #99

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (eba0d26) 33.28% compared to head (53df1bc) 31.49%.

❗ Current head 53df1bc differs from pull request most recent head 612b1aa. Consider uploading reports for the commit 612b1aa to get more accurate results

Files Patch % Lines
...controllers/statusaggregator/plugins/deployment.go 0.00% 2 Missing ⚠️
pkg/util/annotation/annotation.go 88.23% 2 Missing ⚠️
pkg/controllers/nsautoprop/controller.go 0.00% 1 Missing ⚠️
pkg/controllers/status/controller.go 0.00% 1 Missing ⚠️
pkg/controllers/sync/resource.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
- Coverage   33.28%   31.49%   -1.79%     
==========================================
  Files         155      122      -33     
  Lines       17892    14300    -3592     
==========================================
- Hits         5955     4504    -1451     
+ Misses      11427     9391    -2036     
+ Partials      510      405     -105     
Flag Coverage Δ
unittests 31.49% <76.66%> (-1.79%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@z1cheng z1cheng force-pushed the refactor_annotation branch 6 times, most recently from 2b1b473 to a162cdf Compare December 7, 2023 06:16
@z1cheng z1cheng changed the title refactor: remove redundant the returned error refactor: remove redundant returned error Dec 10, 2023
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this IsNilPointer check to expose the problem early.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try removing IsNilPointer check from all the functions in this file. If there's no other references to IsNilReference, we should remove its definition as well.

if key == "" {
return false, fmt.Errorf("key is a empty string.")
func AddAnnotation(obj metav1.Object, key, value string) bool {
if IsNilPointer(obj) || key == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key is usually a constant, so the chance that it's empty is slim. Let's remove this key check to make it clear that it's the caller's responsibility to pass in a valid key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Member

@gary-lgy gary-lgy Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls fix the unit tests. I think we can remove the test cases where an empty key is passed in.

@z1cheng z1cheng force-pushed the refactor_annotation branch 2 times, most recently from 019dafe to 24ce9e4 Compare December 12, 2023 05:44
@z1cheng z1cheng force-pushed the refactor_annotation branch 3 times, most recently from 5cbf6bc to 53df1bc Compare December 14, 2023 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants