fix: return error and stop spinner on domain delete failure#211
fix: return error and stop spinner on domain delete failure#211eliajhmauve wants to merge 1 commit intozeabur:mainfrom
Conversation
Two issues in domain/delete: 1. When RemoveDomain returns false (API-level failure), the code logged a warning and returned nil (exit code 0), making it impossible for callers to detect the failure. 2. Both ListDomains and RemoveDomain error paths returned early without calling s.Stop(), leaving the spinner running and corrupting terminal output. Fix: call s.Stop() before returning errors, and return fmt.Errorf instead of nil when deleteResult is false.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Fixes domain delete CLI behavior so terminal output isn’t corrupted by a lingering spinner on API errors, and so domain deletion failures correctly propagate as errors (non-zero exit).
Changes:
- Stop the spinner before returning on
ListDomains/RemoveDomainerror paths. - Return an error (instead of
nil) when the API reports deletion failed (deleteResult == false).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s := spinner.New(cmdutil.SpinnerCharSet, cmdutil.SpinnerInterval, | ||
| spinner.WithColor(cmdutil.SpinnerColor), | ||
| spinner.WithSuffix(fmt.Sprintf(" Deleting selected domain: %s ...", opts.domainName))) | ||
| s.Start() | ||
| deleteResult, err := f.ApiClient.RemoveDomain(context.Background(), opts.domainName) | ||
| if err != nil { | ||
| s.Stop() | ||
| return err |
There was a problem hiding this comment.
runDeleteDomainNonInteractive can call RemoveDomain with an empty opts.domainName when the command is run non-interactively without --domain, leading to a potentially confusing API error (or unintended server behavior). Add an explicit check (e.g., return a --domain is required error) before starting the spinner / calling the API.
pan93412
left a comment
There was a problem hiding this comment.
You need to resolve Copilot's suggestion (or explain why their suggestion is invalid)
Problem
domain deletehas two related issues inrunDeleteDomainNonInteractive:1. Spinner not stopped on API error
Both
ListDomainsandRemoveDomaincallreturn errbefores.Stop(), leaving the spinner running after an error. This corrupts terminal output — the spinner animation continues rendering on top of the error message.2. Deletion failure returns
nil(exit code 0)When
RemoveDomainreturnsfalse(the API-level "failed" signal), the code logs a warning and returnsnil:Callers (scripts, CI/CD pipelines) have no way to detect that the deletion did not actually succeed.
Fix
s.Stop()before eachreturn errin the error pathsf.Log.Warnf(...); return nilwithreturn fmt.Errorf(...)so failures propagate correctlyFiles changed
internal/cmd/domain/delete/delete.go— 3-line change