Skip to content

[SREP-4895] Fallback to deleted_cluster endpoint to get cluster information. #898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bergmannf wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[SREP-4895] Fallback to deleted_cluster endpoint to get cluster information. #898

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions pkg/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ func GetCluster(connection *sdk.Connection, key string) (cluster *cmv1.Cluster,
// Prepare the resources that we will be using:
subsResource := connection.AccountsMgmt().V1().Subscriptions()
clustersResource := connection.ClustersMgmt().V1().Clusters()
deletedClustersResource := connection.ClustersMgmt().V1().DeletedClusters()

// Try to find a matching subscription:
subsSearch := fmt.Sprintf(
Expand Down Expand Up @@ -146,6 +147,29 @@ func GetCluster(connection *sdk.Connection, key string) (cluster *cmv1.Cluster,
return
}

// 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)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 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.go

Repository: 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.go

Repository: 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.go

Repository: 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.

Copy link
Copy Markdown
Contributor Author

@bergmannf bergmannf May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same capitalization is used as in all other messages - maintaining consistency is probably more useful than fixing just this one occurrence.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

return
}

// If there is exactly one cluster matching then return it:
clustersTotal = deletedClustersResponse.Total()
if clustersTotal == 1 {
cluster = deletedClustersResponse.Items().Slice()[0].Cluster()
return
}

// If there are multiple matching clusters then we should report it as an error:
if clustersTotal > 1 {
err = fmt.Errorf(
"There are %d deleted clusters with identifier or name '%s'",
clustersTotal, key,
)
return
}

// If we are here then there are no subscriptions or clusters matching the passed key:
err = fmt.Errorf(
"There are no subscriptions or clusters with identifier or name '%s'",
Expand Down