[SREP-4895] Fallback to deleted_cluster endpoint to get cluster information.#898
[SREP-4895] Fallback to deleted_cluster endpoint to get cluster information.#898bergmannf wants to merge 1 commit into
Conversation
WalkthroughThe PR extends the ChangesDeleted Cluster Lookup Fallback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bergmannf The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/utils/utils.go`:
- Line 153: Change the non-idiomatic capitalized error messages and switch to
proper Go error wrapping: replace "Can't retrieve deleted clusters for key '%s':
'%v'" with a lowercase message and use %w to wrap the underlying error (e.g.,
"can't retrieve deleted clusters for key %q: %w"), and make the same adjustments
for the other occurrences of "Can't retrieve..." in this file (the instances at
the other locations with the same pattern should be lowercased and use %w
instead of %v) so error wrapping and ST1005 style are satisfied.
🪄 Autofix (Beta)
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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6c61175d-a80a-4fc7-87ca-edb64bfa0bfb
📒 Files selected for processing (1)
pkg/utils/utils.go
| // If we get here we might still be able to get some information from the deleted_clusters information: | ||
| deletedClustersResponse, err := deletedClustersResource.List().Search(clustersSearch).Size(1).Send() | ||
| if err != nil { | ||
| err = fmt.Errorf("Can't retrieve deleted clusters for key '%s': '%v'", key, err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify non-idiomatic capitalized fmt.Errorf strings and %v wrapping in this file
rg -n 'fmt\.Errorf\("[A-Z]' pkg/utils/utils.go
rg -n 'fmt\.Errorf\([^"]*%v' pkg/utils/utils.goRepository: openshift/osdctl
Length of output: 383
🏁 Script executed:
# Check the exact content around line 153 and line 166
sed -n '150,170p' pkg/utils/utils.goRepository: openshift/osdctl
Length of output: 820
🏁 Script executed:
# Get full context of all fmt.Errorf calls to understand if others need fixing too
sed -n '83p;130p;153p;166,167p;189p' pkg/utils/utils.goRepository: openshift/osdctl
Length of output: 453
Fix non-idiomatic error message capitalization and use proper error wrapping in fmt.Errorf calls.
Multiple fmt.Errorf calls in this file violate ST1005. Line 153 should use %w instead of %v for error wrapping, and both lines 153 and 166 use capitalized error messages that should be lowercase. Similar issues appear at lines 83 and 130.
Proposed fix
- err = fmt.Errorf("Can't retrieve deleted clusters for key '%s': '%v'", key, err)
+ err = fmt.Errorf("can't retrieve deleted clusters for key %q: %w", key, err)
@@
- "There are %d deleted clusters with identifier or name '%s'",
+ "there are %d deleted clusters with identifier or name '%s'",Also applies to: 83, 130 (same pattern: "Can't retrieve..." with %v instead of %w)
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 153-153: ST1005: error strings should not be capitalized
(staticcheck)
🤖 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 `@pkg/utils/utils.go` at line 153, Change the non-idiomatic capitalized error
messages and switch to proper Go error wrapping: replace "Can't retrieve deleted
clusters for key '%s': '%v'" with a lowercase message and use %w to wrap the
underlying error (e.g., "can't retrieve deleted clusters for key %q: %w"), and
make the same adjustments for the other occurrences of "Can't retrieve..." in
this file (the instances at the other locations with the same pattern should be
lowercased and use %w instead of %v) so error wrapping and ST1005 style are
satisfied.
There was a problem hiding this comment.
Same capitalization is used as in all other messages - maintaining consistency is probably more useful than fixing just this one occurrence.
There was a problem hiding this comment.
@bergmannf, that's a fair point — fixing only the newly added line while leaving the same pattern at lines 83 and 130 unchanged would indeed create inconsistency.
Would you like me to open a follow-up issue to track a style cleanup across the whole file (lowercasing error strings and switching %v to %w where appropriate), so it can be addressed holistically in a dedicated PR?
|
/retest |
|
@bergmannf: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 participant
Summary by CodeRabbit