Fix cluster quota handler registration - #210
Conversation
|
Hi @vishnubijukumar. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Instead of panicking with a fixed string in the controller constructors, consider including the underlying error in the panic message (or returning it) to aid debugging when handler registration fails.
- The
erroringSharedIndexInformertest helper is duplicated in both constructor test files; consider moving it into a shared test utility to avoid repetition and keep behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of panicking with a fixed string in the controller constructors, consider including the underlying error in the panic message (or returning it) to aid debugging when handler registration fails.
- The `erroringSharedIndexInformer` test helper is duplicated in both constructor test files; consider moving it into a shared test utility to avoid repetition and keep behavior consistent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Vishnu Bijukumar <vishnubijukumar@ibm.com>
Signed-off-by: Vishnu Bijukumar <vishnubijukumar@ibm.com>
9333b4b to
ae9536e
Compare
What this PR does / why we need it:
Fail fast when cluster quota controller event handler registration fails, instead of silently continuing with a partially initialized controller.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
This updates the CRQ and AACRQ controller constructors to stop ignoring informer handler registration errors.
It also adds focused constructor tests for both controllers and fixes adjacent Infof usage in the touched files so the package tests pass.
Release note: