fix(rbg): use structured logging instead of event recorder in Get fai…#137
fix(rbg): use structured logging instead of event recorder in Get fai…#137Syspretor merged 2 commits intosgl-project:mainfrom
Conversation
…lure case This commit fixes an issue where the controller attempted to record an event on a potentially incomplete RBG object when Get() fails. Changes: - Initialize logger at the beginning of Reconcile function - Replace event recorder with structured logging when Get fails - Use logger.Error() with name and namespace fields for better traceability - Optimize logger initialization to avoid duplication - Use IsZero() instead of nil check for DeletionTimestamp (linter improvement) Rationale: When r.client.Get() fails, the rbg object may be empty or incomplete, lacking necessary metadata (Name, Namespace, UID). Recording an event on such an object could fail or create events that cannot be properly associated with the resource. Using structured logging is more appropriate for this error scenario and aligns with other controllers in the codebase (e.g., RoleBasedGroupSet controller). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Summary of ChangesHello @jackyzzy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the error handling and logging mechanisms within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly replaces the event recorder with structured logging for handling Get failures, which is a more robust approach when the resource object might be incomplete. The change to use IsZero() for DeletionTimestamp checks is also a good improvement. The overall changes enhance the controller's reliability and logging clarity. I have one suggestion to further refine the error logging to differentiate NotFound errors from other failures, which is a common best practice in Kubernetes controllers.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Distinguish between NotFound errors and actual errors (network, permissions) - Log NotFound as Info level since object might be deleted - Log actual errors as Error level and return them for retry - Remove client.IgnoreNotFound() to properly handle different error types Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
/lgtm |
|
@jackyzzy Thanks for this contribution! |
3 participants
…lure case
This commit fixes an issue where the controller attempted to record an event on a potentially incomplete RBG object when Get() fails.
Changes:
Rationale:
When r.client.Get() fails, the rbg object may be empty or incomplete, lacking necessary metadata (Name, Namespace, UID). Recording an event on such an object could fail or create events that cannot be properly associated with the resource. Using structured logging is more appropriate for this error scenario and aligns with other controllers in the codebase (e.g., RoleBasedGroupSet controller).
🤖 Generated with Claude Code
Ⅰ. Motivation
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.