Conversation
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Prevent accidental service deletion in non-interactive mode (e.g. CI/CD) by requiring explicit --yes confirmation, consistent with project delete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThe pull request introduces a new service deletion feature by adding a delete command handler, registering it with the service command group, and implementing the underlying API interface and client method to support deleting services by ID within a specified environment. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a new zeabur service delete CLI command and wires it into the existing service command group, backed by a new DeleteService GraphQL mutation on the API client. It also introduces a --yes guard in non-interactive mode to reduce the chance of accidental deletions.
Changes:
- Add
DeleteServiceto the API client and public API interface. - Register a new
zeabur service deletesubcommand underzeabur service. - Implement interactive selection + confirmation flow, and require
--yesin non-interactive mode.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/api/service.go | Adds DeleteService GraphQL mutation call. |
| pkg/api/interface.go | Exposes DeleteService on the API interface. |
| internal/cmd/service/service.go | Registers the new service delete command. |
| internal/cmd/service/delete/delete.go | Implements the delete command behavior (interactive + non-interactive). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } else if !f.Interactive && !opts.skipConfirm { | ||
| f.Log.Info("Please use --yes to confirm deletion without interactive prompt") | ||
| return nil |
There was a problem hiding this comment.
In non-interactive mode, missing --yes currently logs an info message and returns nil, which exits with status 0. For scripting/CI safety, consider returning a non-nil error here so callers can reliably detect that the delete was refused (while still printing the guidance to use --yes).
| return nil | |
| return fmt.Errorf("deletion not confirmed: rerun with --yes to confirm deletion in non-interactive mode") |
There was a problem hiding this comment.
Keeping return nil to stay consistent with the existing project delete command, which uses the same pattern. Changing the exit behavior for just this command would introduce inconsistency.
| if err != nil { | ||
| return err | ||
| } | ||
| if !confirm { |
There was a problem hiding this comment.
When the user declines the confirmation prompt, the command returns without any message. Consider logging a clear cancellation message (consistent with other delete commands) so users understand no action was taken.
| if !confirm { | |
| if !confirm { | |
| f.Log.Info("Service deletion cancelled") |
|
|
||
| // double check | ||
| if f.Interactive && !opts.skipConfirm { | ||
| confirm, err := f.Prompter.Confirm(fmt.Sprintf("Are you sure to delete service <%s>?", idOrName), false) |
There was a problem hiding this comment.
The confirmation prompt text is a bit awkward ("Are you sure to delete..."). Consider rephrasing to something like "Are you sure you want to delete service <...>?" for clarity.
| confirm, err := f.Prompter.Confirm(fmt.Sprintf("Are you sure to delete service <%s>?", idOrName), false) | |
| confirm, err := f.Prompter.Confirm(fmt.Sprintf("Are you sure you want to delete service <%s>?", idOrName), false) |
![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
🤖 Fix all issues with AI agents
In `@internal/cmd/service/delete/delete.go`:
- Around line 98-101: The branch that currently logs "Please use --yes to
confirm deletion without interactive prompt" should return a non-nil error to
produce a non-zero exit code instead of returning nil; update the conditional
using f.Interactive and opts.skipConfirm so it returns an error (e.g., a
formatted error or cli.Exit) after logging via f.Log.Info (or replace the log
with f.Log.Error) with a clear message like "use --yes to confirm deletion
without interactive prompt" so scripts detect failure. Ensure you modify the
function that contains the f.Interactive / opts.skipConfirm check to propagate
the error return type.
| } else if !f.Interactive && !opts.skipConfirm { | ||
| f.Log.Info("Please use --yes to confirm deletion without interactive prompt") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Return a non-zero exit when --yes is required but missing.
Right now the command logs a message and exits successfully even though nothing happened. That’s risky for scripts that assume success means deletion.
💡 Proposed fix
- } else if !f.Interactive && !opts.skipConfirm {
- f.Log.Info("Please use --yes to confirm deletion without interactive prompt")
- return nil
- }
+ } else if !f.Interactive && !opts.skipConfirm {
+ return fmt.Errorf("confirmation required: re-run with --yes to delete non-interactively")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if !f.Interactive && !opts.skipConfirm { | |
| f.Log.Info("Please use --yes to confirm deletion without interactive prompt") | |
| return nil | |
| } | |
| } else if !f.Interactive && !opts.skipConfirm { | |
| return fmt.Errorf("confirmation required: re-run with --yes to delete non-interactively") | |
| } |
🤖 Prompt for AI Agents
In `@internal/cmd/service/delete/delete.go` around lines 98 - 101, The branch that
currently logs "Please use --yes to confirm deletion without interactive prompt"
should return a non-nil error to produce a non-zero exit code instead of
returning nil; update the conditional using f.Interactive and opts.skipConfirm
so it returns an error (e.g., a formatted error or cli.Exit) after logging via
f.Log.Info (or replace the log with f.Log.Error) with a clear message like "use
--yes to confirm deletion without interactive prompt" so scripts detect failure.
Ensure you modify the function that contains the f.Interactive /
opts.skipConfirm check to propagate the error return type.
There was a problem hiding this comment.
Keeping return nil to stay consistent with project delete, which uses the same pattern (internal/cmd/project/delete/delete.go:61).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the project delete command to verify the pattern
cat internal/cmd/project/delete/delete.go | head -70 | tail -20
Length of output: 471
@canyugs Confirmed! I can see the same pattern in project delete where it returns nil with an info message when --yes is missing. Consistency across commands makes sense. Thanks for the context! 👍
1 participant
Summary
zeabur service deletecommand to delete a service from an environmentDeleteServiceGraphQL mutation to the API client--yesflag in non-interactive mode to prevent accidental deletionsTest plan
zeabur service deletein interactive mode — should prompt for service selection and confirmationzeabur service delete --id <id> --env-id <env>without--yes— should refuse and prompt to use--yeszeabur service delete --id <id> --env-id <env> --yes— should delete successfully🤖 Generated with Claude Code
Summary by CodeRabbit