✨ feat(audit): improve logging in export dir - #765
Open
Tamar-Dinavetsky wants to merge 10 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe export command now logs lifecycle events, validation and discovery failures, resource counts, RBAC filtering, timeout errors, all-Forbidden results, and completion. Error formatting uses ChangesExport observability
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/export/export.go (1)
214-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a consistent error verb for logging.
These new error logs use
%vfor the error value. Other error logs in this function, for example at Line 200 and Line 210, use%#v. Use one verb consistently across the function for error logging.♻️ Proposed fix for consistent error formatting
- log.Errorf("namespace validation failed: %v", err) + log.Errorf("namespace validation failed: %#v", err)- log.Errorf("cannot discover preferred resources: %v", err) + log.Errorf("cannot discover preferred resources: %#v", err)- log.Errorf("timeout listing resource %q: %v", resErr.APIResource.Kind, resErr.Error) + log.Errorf("timeout listing resource %q: %#v", resErr.APIResource.Kind, resErr.Error)Also applies to: 255-255, 279-279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/export/export.go` at line 214, Update the error log calls in the export function, including the messages for namespace validation and the other indicated errors, to use the same %#v formatting verb already used by nearby error logs instead of %v. Keep the existing log messages and error handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/export/export.go`:
- Line 214: Update the error log calls in the export function, including the
messages for namespace validation and the other indicated errors, to use the
same %#v formatting verb already used by nearby error logs instead of %v. Keep
the existing log messages and error handling unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/export/export.go (1)
213-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the namespace to the validation failure log.
The log message "namespace validation failed: %v" does not include
o.userSpecifiedNamespace. Add the namespace value so the log entry is self-contained and actionable without cross-referencing the earlier "Starting export" log line.🛠️ Proposed fix
if err := validateExportNamespace(context.Background(), kubeClient, o.userSpecifiedNamespace, log); err != nil { - log.Errorf("namespace validation failed: %v", err) + log.Errorf("namespace validation failed for namespace %q: %v", o.userSpecifiedNamespace, err) return err }As per coding guidelines, "Do not skip error context - include resource names and types."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/export/export.go` around lines 213 - 216, Update the validation failure log in the export flow around validateExportNamespace to include o.userSpecifiedNamespace alongside the existing error details, making the entry self-contained while preserving the current error return behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/export/export.go`:
- Around line 213-216: Update the validation failure log in the export flow
around validateExportNamespace to include o.userSpecifiedNamespace alongside the
existing error details, making the entry self-contained while preserving the
current error return behavior.
Tamar-Dinavetsky
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/export/cluster.go`:
- Around line 240-252: Update the acceptance logs in the ServiceAccount branch,
ClusterRole acceptance path, and direct SCC RoleRef path to include the specific
match path: ServiceAccount namespace/name, ClusterRoleBinding name, or SCC
RoleRef details. Follow the existing context format used by the Group, User, and
SCC-via-ClusterRoleBinding logs, while preserving the current acceptance
behavior.
- Line 229: Update the conversion warnings in the ClusterRoleBinding,
ClusterRole, and SCC paths to include the source object's GetKind(), GetName(),
actual %T, and API resource name alongside the error. Use the f variable when
formatting nested ClusterRoleBinding objects in the ClusterRole and SCC paths,
and apply the same context to the warnings around lines 266-273 and 290-299.
In `@cmd/export/discover.go`:
- Around line 289-301: Update all listing failure logs in the surrounding
error-handling switch, including the timeout log before it, to include
g.APIResource.Name alongside groupVersion and kind. Preserve the existing
severity levels and messages while ensuring timeout, forbidden,
unsupported-method, not-found, and generic errors identify the resource name.
- Line 391: Update the four type-assertion error returns in the resource
discovery flow near the existing logger.Errorf call to include API resource
name, group version, kind, and received type. Wrap the iteration error at the
first return with %w while preserving the underlying error, and apply the same
resource context to the returns at the other three locations.
- Around line 302-305: Preserve the original per-item GET error through the
aggregate error path so resourceToExtract can detect Kubernetes API timeouts and
context deadline exceeded with errors.Is. Update iterateItemsByGet/EachListItem
error aggregation and groupResourceError formatting to wrap the underlying error
rather than stringify it, then add a regression test covering per-item GET
timeouts for imagestreamtags or imagetags.
- Around line 225-227: Update the ServerPreferredResources error-handling flow
to log an error before returning when discovery.IsGroupDiscoveryFailedError has
no resource lists. Preserve the existing warning for partial discovery failures
and the existing error log for other failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a4de162-4cd0-43d2-83b3-0771939e51a7
📒 Files selected for processing (4)
cmd/export/cluster.gocmd/export/crd.gocmd/export/discover.gocmd/export/export.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/export/export.go
Tamar-Dinavetsky
changed the title
Contributor
Author
|
/rfr |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#773
Adds missing log statements to the export command:
detection)
Summary by CodeRabbit
Summary by CodeRabbit