Conversation
acarranoqovery
commented
Mar 10, 2026
acarranoqovery
commented
- fixed command examples
- added informations on the deleted accounts
…n flow - Fix command path (remove spurious 'organization' subcommand in help examples) - Show org name and cluster details before confirmation - Dry-run now prompts for confirmation but skips actual deletion - With --disable-dry-run, skips prompt and proceeds with deletion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Guimove
left a comment
Guimove
left a comment
There was a problem hiding this comment.
Complete review — details in inline comments.
| @@ -33,7 +64,15 @@ func DeleteOrganizations(organizationIds []string, allowFailedClusters bool, dry | |||
| os.Exit(1) | |||
There was a problem hiding this comment.
🔴 Blocking bug — security regression
Validate was moved inside the !dryRunDisabled block, meaning the confirmation prompt only runs in dry-run mode. When the user passes --disable-dry-run to perform real deletions, the code skips utils.Validate entirely and proceeds straight to the HTTP DELETE.
This is a regression introduced by this PR (verifiable on commit 7284c0d where Validate was called unconditionally before any dry-run check).
Suggestion:
if !utils.Validate("delete") {
return
}
if !dryRunDisabled {
log.Info("Dry run: no deletion performed")
return
}| client := utils.GetQoveryClient(tokenType, token) | ||
|
|
||
| for _, orgId := range organizationIds { | ||
| org, _, err := client.OrganizationMainCallsAPI.GetOrganization(context.Background(), orgId).Execute() |
There was a problem hiding this comment.
🟡 No timeout on API calls
context.Background() is used without a deadline — if the API is slow or unreachable, the preview hangs indefinitely with no feedback to the user.
Suggestion:
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
org, _, err := client.OrganizationMainCallsAPI.GetOrganization(ctx, orgId).Execute()| } else { | ||
| fmt.Printf(" - %s (%s)\n", orgId, org.Name) | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 No pagination on cluster list
GetResults() only returns the first page. An organization with many clusters will show an incomplete list, which could mislead the operator before a destructive action.
At minimum, add a warning if there are more pages, or implement full pagination.
|
|
||
| func printOrganizationsPreview(organizationIds []string) { | ||
| tokenType, token, err := utils.GetAccessToken() | ||
| if err != nil { |
There was a problem hiding this comment.
🟡 Double token acquisition
printOrganizationsPreview calls GetAccessToken() here, and deleteWithBody will call it again later. For long previews (many orgs), the token could expire between the two calls, causing an opaque failure on the actual deletion.
Suggestion: fetch the token once in DeleteOrganizations and pass the client down as a parameter.
| if err != nil { | ||
| fmt.Printf(" - %s (name: unknown)\n", orgId) | ||
| } else { | ||
| fmt.Printf(" - %s (%s)\n", orgId, org.Name) |
There was a problem hiding this comment.
🟢 Inconsistent indentation in output
2 spaces for orgs, 6 spaces for clusters. Not aligned with the rest of the codebase (e.g. displayDeletionResults uses 2 spaces consistently).
Suggestion:
fmt.Printf(" - cluster: %s (%s)\n", c.Id, c.Name)| @@ -33,7 +64,15 @@ func DeleteOrganizations(organizationIds []string, allowFailedClusters bool, dry | |||
| os.Exit(1) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
🟢 Mixed log.Info vs fmt.Printf for user-facing output
The preview uses fmt.Printf (always printed) but the dry-run message uses log.Info (subject to logrus log level, suppressed if level > INFO). Use fmt.Printf or utils.PrintlnInfo consistently throughout.
| allowFailedClusters bool | ||
| organizationIds []string | ||
| allowFailedClusters bool | ||
|
|
There was a problem hiding this comment.
🟢 Missing period in Long: description
Clusters must be deleted before via the force-delete-cluster command
Missing a period at the end of the sentence before the newline.
2 participants
