fix: return error on domain create when domain is unavailable#217
fix: return error on domain create when domain is unavailable#217eliajhmauve wants to merge 1 commit intozeabur:mainfrom
Conversation
domain create logs a warning but returns nil (exit 0) when the domain is not available, causing CI/CD pipelines to think the domain was created successfully. Return an error instead. Also add missing s.Stop() before early error return to prevent the spinner from continuing after failure.
WalkthroughThe domain creation flow now properly handles unavailable domain errors by explicitly stopping the spinner and returning an error instead of logging a warning and continuing execution, changing from non-fatal to fatal error handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
internal/cmd/domain/create/create.go (1)
126-130: Consider stopping spinner on error for consistency.The same pattern fixed at line 112 exists here: if
ListDomainsfails, the spinner continues running. For consistency with the fix applied above, consider stopping the spinner before returning.♻️ Suggested fix
existedDomains, err := f.ApiClient.ListDomains(context.Background(), opts.id, opts.environmentID) if err != nil { + s.Stop() return err }The same pattern also exists at lines 186-188 in
runCreateDomainNonInteractive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/domain/create/create.go` around lines 126 - 130, When ListDomains fails the spinner `s` is not stopped, causing it to keep running; update the error paths that call `f.ApiClient.ListDomains` (in the interactive create flow and in `runCreateDomainNonInteractive`) to call `s.Stop()` immediately before returning the error so the spinner is always stopped on failure; locate the `s` spinner variable and add `s.Stop()` just prior to each `return err` after `ListDomains` calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cmd/domain/create/create.go`:
- Around line 126-130: When ListDomains fails the spinner `s` is not stopped,
causing it to keep running; update the error paths that call
`f.ApiClient.ListDomains` (in the interactive create flow and in
`runCreateDomainNonInteractive`) to call `s.Stop()` immediately before returning
the error so the spinner is always stopped on failure; locate the `s` spinner
variable and add `s.Stop()` just prior to each `return err` after `ListDomains`
calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b4abb6e-4a51-4cc4-b50e-6a64f7d46a1b
📒 Files selected for processing (1)
internal/cmd/domain/create/create.go
Summary
domain createlogs a warning but returnsnil(exit 0) when the domain is not available, causing CI/CD pipelines to think the domain was created successfully.Bug
Fix
fmt.Errorf(...)instead ofnilso the caller gets a non-zero exit codes.Stop()before early error return to prevent spinner from continuing after failureSummary by CodeRabbit
Bug Fixes