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 unnecessary AddEventHandler error handling #12917

Merged

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Apr 9, 2024

Fixes #12847 (comment)

Motivation

  • in k8s 1.26+, the informer.AddEventHandler function can return an error
    • however, this error will only happen when the informer has been stopped already
    • in the new*Informer functions, we literally just created them, with AddEventHandler on the very next line, so this error is impossible in these cases
      • so ignore the error and remove some error handling code around that

Modifications

  • add //nolint:errcheck directives with comments on errors that can be safely ignored
    • remove unnecessary error handling code
  • remove some extra new lines

Verification

lint passes, all E2Es pass. checked the informer source code myself.

- in k8s 1.26+, the `informer.AddEventHandler` function can return an error
  - however, this error will only happen when the informer been stopped already (https://github.com/kubernetes/client-go/blob/46588f2726fa3e25b1704d6418190f424f95a990/tools/cache/shared_informer.go#L580)
  - in the `new*Informer` functions, we literally just created them, with `AddEventHandler` on the very next line, so this error is impossible in these cases
    - so ignore the error and remove some error handling code around that

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added the area/controller Controller issues, panics label Apr 9, 2024
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Copy link
Contributor

@blkperl blkperl left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up!

@isubasinghe isubasinghe merged commit 87a2041 into argoproj:main Apr 11, 2024
27 checks passed
@agilgur5 agilgur5 deleted the refactor-remove-unnecessary-error-handling branch April 11, 2024 13:43
@agilgur5 agilgur5 added this to the v3.6.0 milestone Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics type/tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants