Fix type assertion error messages in export discover#197
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBoth iteration helpers in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/export/discover.go`:
- Around line 286-287: The logger.Errorf calls in the two type-assertion failure
branches (the ones that currently pass g and object but use three format verbs)
have a format/argument mismatch and pass g where the object's runtime type is
expected; update both logger.Errorf invocations to use three verbs and pass the
arguments in the correct order so the object's type is logged first, then the
groupResource, then the object value (e.g. use a format like "expected
unstructured.Unstructured but got %T for groupResource %v and object: %#v" and
pass object, g, object to logger.Errorf in both branches).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
When type assertions failed, error messages displayed the nil pointer variable instead of the actual object type, making debugging difficult. Now error messages correctly show the actual type received. Also fixed logger.Errorf format string mismatch where 3 format verbs were used but only 2 arguments provided. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
istein1
force-pushed
the
fix/type-assertion-error-messages
branch
from
bb6a29e to
4b1d0e1
Compare
aufi
left a comment
aufi
left a comment
There was a problem hiding this comment.
LGTM
Just added a small comment, but no problem with merging.
| // TODO: explore aggregating all the errors here instead of terminating the loop | ||
| logger.Errorf("expected unstructured.Unstructured but got %T for groupResource %s and object: %#v\n", g, object) | ||
| return fmt.Errorf("expected *unstructured.Unstructured but got %T", u) | ||
| logger.Errorf("expected unstructured.Unstructured but got %T for groupResource %s and object: %#v\n", object, g.APIResource.Name, object) |
There was a problem hiding this comment.
From this function point of view, g could be nil (after de-dereference, so g.APIResource.. could fail), but the rest of code doesn't look to make that possible, so just a note.
Add defensive nil checks in iterateItemsByGet and iterateItemsInList to prevent potential nil pointer dereference of g.APIResource. Addresses code review feedback on PR #197. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
4 participants
Summary
Fixed a bug where type assertion failure error messages displayed the nil pointer variable instead of the actual object type, making debugging difficult.
Changes:
cmd/export/discover.go:287- IniterateItemsByGet(), changed error message to useobjectinstead ofucmd/export/discover.go:310- IniterateItemsInList(), changed error message to useobjectinstead ofuBefore: When type assertion failed, error would show:
After: Error now correctly shows the actual type:
Impact
This is a low-severity bug that affects error message quality. When Kubernetes API returns an unexpected type during export operations, developers will now see helpful error messages indicating the actual type received, rather than confusing nil pointer messages.
Test plan
go build ./cmd/export/...🤖 Generated with Claude Code
Summary by CodeRabbit