Skip to content

chore: improve delete orga command #616

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
acarranoqovery wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chore: improve delete orga command #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2295bc0
fix: use right command sintax
acarranoqovery Mar 10, 2026
175e48e
fix(admin): improve delete org command UX and fix dry-run confirmatio…
acarranoqovery Mar 10, 2026
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
18 changes: 9 additions & 9 deletions cmd/admin_delete_orga.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,32 @@ import (
)

var (
organizationIds []string
allowFailedClusters bool
organizationIds []string
allowFailedClusters bool

Copy link
Contributor

@Guimove Guimove Mar 18, 2026

Choose a reason for hiding this comment

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

🟢 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.

adminDeleteOrgaCmd = &cobra.Command{
Use: "delete",
Short: "Delete one or more organizations by their IDs",
Long: `Delete one or more organizations by providing their IDs.
Long: `Delete one or more organizations by providing their IDs. Clusters must be deleted before via the force-delete-cluster command

Examples:
# Delete a single organization
qovery admin organization delete --organization-id org-123
qovery admin delete --organization-id org-123

# Delete multiple organizations (comma-separated)
qovery admin organization delete --organization-id "org-123,org-456,org-789"
qovery admin delete --organization-id "org-123,org-456,org-789"

# Delete multiple organizations (repeated flag)
qovery admin organization delete --organization-id org-123 --organization-id org-456
qovery admin delete --organization-id org-123 --organization-id org-456

# Mix both formats
qovery admin organization delete -o "org-123,org-456" -o org-789
qovery admin delete -o "org-123,org-456" -o org-789

# Allow deletion of organizations with failed clusters
qovery admin organization delete -o org-123 --allow-failed-clusters
qovery admin delete -o org-123 --allow-failed-clusters

# Disable dry-run to actually delete
qovery admin organization delete -o org-123 --disable-dry-run`,
qovery admin delete -o org-123 --disable-dry-run`,
Run: func(cmd *cobra.Command, args []string) {
deleteOrganizations()
},
Expand Down
49 changes: 40 additions & 9 deletions pkg/delete_orga.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package pkg

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
Expand All @@ -13,6 +14,36 @@ import (
"github.com/qovery/qovery-cli/utils"
)

func printOrganizationsPreview(organizationIds []string) {
tokenType, token, err := utils.GetAccessToken()
if err != nil {
Copy link
Contributor

@Guimove Guimove Mar 18, 2026

Choose a reason for hiding this comment

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

🟡 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.

log.Warnf("Could not fetch organization details: %v", err)
for _, id := range organizationIds {
fmt.Printf(" - %s\n", id)
}
return
}
client := utils.GetQoveryClient(tokenType, token)

for _, orgId := range organizationIds {
org, _, err := client.OrganizationMainCallsAPI.GetOrganization(context.Background(), orgId).Execute()
Copy link
Contributor

@Guimove Guimove Mar 18, 2026

Choose a reason for hiding this comment

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

🟡 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()

if err != nil {
fmt.Printf(" - %s (name: unknown)\n", orgId)
} else {
fmt.Printf(" - %s (%s)\n", orgId, org.Name)
Copy link
Contributor

@Guimove Guimove Mar 18, 2026

Choose a reason for hiding this comment

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

🟢 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)

}

Copy link
Contributor

@Guimove Guimove Mar 18, 2026

Choose a reason for hiding this comment

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

🟡 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.

clusters, _, err := client.ClustersAPI.ListOrganizationCluster(context.Background(), orgId).Execute()
if err != nil {
log.Warnf("Could not fetch clusters for organization %s: %v", orgId, err)
} else if clusters != nil {
for _, c := range clusters.GetResults() {
fmt.Printf(" cluster: %s (%s)\n", c.Id, c.Name)
}
}
}
}

type DeleteOrganizationsResponse struct {
Deleted []string `json:"deleted"`
Failed []DeleteOrganizationFailure `json:"failed"`
Expand All @@ -33,7 +64,15 @@ func DeleteOrganizations(organizationIds []string, allowFailedClusters bool, dry
os.Exit(1)
Copy link
Contributor

@Guimove Guimove Mar 18, 2026

Choose a reason for hiding this comment

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

🔴 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
}

}

Copy link
Contributor

@Guimove Guimove Mar 18, 2026

Choose a reason for hiding this comment

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

🟢 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.

if !utils.Validate("delete") {
fmt.Printf("The following %d organization(s) will be deleted (allowFailedClusters=%t):\n", len(organizationIds), allowFailedClusters)
printOrganizationsPreview(organizationIds)
fmt.Println()

if !dryRunDisabled {
if !utils.Validate("delete") {
return
}
log.Info("Dry run: no deletion performed")
return
}

Expand All @@ -49,14 +88,6 @@ func DeleteOrganizations(organizationIds []string, allowFailedClusters bool, dry
log.Fatalf("Failed to marshal organization IDs: %v", err)
}

if !dryRunDisabled {
fmt.Printf("Would delete %d organization(s) (allowFailedClusters=%t):\n", len(organizationIds), allowFailedClusters)
for _, id := range organizationIds {
fmt.Printf(" - %s\n", id)
}
return
}

// Make HTTP request
res := deleteWithBody(url, http.MethodDelete, true, bytes.NewReader(body))
if res == nil {
Expand Down
Loading