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

Remove always-nil error returns #36910

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Remove always-nil error returns #36910

merged 1 commit into from
Jan 10, 2025

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Jan 9, 2025

These errors are always nil, so they can be omitted and get rid of the respective error checking code. Found using unparam. Note that we cannot enable the unparam linter yet in golangci-lint because there are some false positives.

@tklauser tklauser added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Jan 9, 2025
@tklauser tklauser requested review from a team as code owners January 9, 2025 09:26
@tklauser
Copy link
Member Author

tklauser commented Jan 9, 2025

/test

@github-actions github-actions bot added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. cilium-cli This PR contains changes related with cilium-cli labels Jan 9, 2025
@tklauser tklauser enabled auto-merge January 9, 2025 09:27
Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@Artyop Artyop left a comment

Choose a reason for hiding this comment

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

Just a question for my own curiosity, otherwise lgtm

daemon/cmd/deletion_queue.go Show resolved Hide resolved
@tklauser tklauser force-pushed the pr/tklauser/always-nil-err branch from a992790 to d8aa0e3 Compare January 9, 2025 10:17
@tklauser
Copy link
Member Author

tklauser commented Jan 9, 2025

/test

These errors are always nil, so they can be omitted and get rid of the
respective error checking code. Found using unparam. Note that we cannot
enable the unparam linter yet in golangci-lint because there are some
false positives.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/always-nil-err branch from d8aa0e3 to 7a193c6 Compare January 9, 2025 10:27
@tklauser
Copy link
Member Author

tklauser commented Jan 9, 2025

Fixed up #36910 (comment) and a comment in (*deletionQueue).lock in the latest push, as suggested by Marco.

Incremental diff:

diff --git a/daemon/cmd/deletion_queue.go b/daemon/cmd/deletion_queue.go
index 3724b4a01e43..44687c98a053 100644
--- a/daemon/cmd/deletion_queue.go
+++ b/daemon/cmd/deletion_queue.go
@@ -44,7 +44,6 @@ func (dq *deletionQueue) Start(cell.HookContext) error {
                }
 
                if err := dq.lock(d.ctx); err != nil {
-                       log.WithError(err).Error("deletionQueue: lock failed")
                        return
                }
 
@@ -77,8 +76,8 @@ func (dq *deletionQueue) lock(ctx context.Context) error {
                return err
        }
 
-       // Don't a non-nil return error from here on so successive dq.processQueuedDeletes can still
-       // continue with best effort.
+       // Don't return a non-nil error from here on so a successive call to dq.processQueuedDeletes
+       // can still continue with best effort.
        var err error
        dq.lf, err = lockfile.NewLockfile(defaults.DeleteQueueLockfile)
        if err != nil {

@tklauser
Copy link
Member Author

tklauser commented Jan 9, 2025

/test

Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Looks good for my codeowners, thanks!

daemon/cmd/deletion_queue.go Show resolved Hide resolved
@tklauser tklauser requested a review from aanm January 9, 2025 13:26
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

Thanks!

@tklauser tklauser added this pull request to the merge queue Jan 10, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 10, 2025
Merged via the queue into main with commit d04477a Jan 10, 2025
293 of 294 checks passed
@tklauser tklauser deleted the pr/tklauser/always-nil-err branch January 10, 2025 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cilium-cli This PR contains changes related with cilium-cli kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants