Exit non-zero when dapr stop or dapr uninstall fails#1668
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Dapr CLI so that dapr stop and dapr uninstall return a non-zero exit status when the underlying operation fails, enabling shell scripts and CI pipelines to reliably detect failure conditions.
Changes:
dapr stop: exits with status 1 on run-file stop failures, run-template parse errors (with-k), Kubernetes stop failures, and when any--app-idstop operation fails.dapr uninstall: exits with status 1 when the uninstall operation returns an error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/stop.go | Adds explicit non-zero exits for multiple failure paths and tracks per-app stop failures before exiting 1. |
| cmd/uninstall.go | Exits non-zero when uninstall fails instead of printing an error and returning success. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| err = executeStopWithRunFile(runFilePath) | ||
| if err != nil { | ||
| print.FailureStatusEvent(os.Stderr, "Failed to stop Dapr and app processes: %s", err) | ||
| } else { | ||
| print.SuccessStatusEvent(os.Stdout, "Dapr and app processes stopped successfully") | ||
| os.Exit(1) | ||
| } | ||
| print.SuccessStatusEvent(os.Stdout, "Dapr and app processes stopped successfully") | ||
| return |
There was a problem hiding this comment.
Good call - and CI actually caught this immediately: the existing stop_without_install e2e was asserting the old behavior (error message + exit 0). I flipped that assertion to require a non-zero exit in a369916, so it now doubles as the regression test for this path.
| if err != nil { | ||
| print.FailureStatusEvent(os.Stderr, fmt.Sprintf("Error removing Dapr: %s", err)) | ||
| } else { | ||
| print.SuccessStatusEvent(os.Stdout, "Dapr has been removed successfully") | ||
| os.Exit(1) | ||
| } | ||
| print.SuccessStatusEvent(os.Stdout, "Dapr has been removed successfully") |
There was a problem hiding this comment.
I looked into this - there isn't a clean way to force standalone.Uninstall or kubernetes.Uninstall to fail from the e2e harness today without mocking, so uninstall doesn't have a dedicated exit-code test yet. If there's a preferred fault-injection pattern in this repo I'm happy to add one.
|
The initial CI run failed for two reasons, both addressed in a369916:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1668 +/- ##
==========================================
+ Coverage 15.57% 16.91% +1.34%
==========================================
Files 64 65 +1
Lines 7244 7330 +86
==========================================
+ Hits 1128 1240 +112
+ Misses 6028 6002 -26
Partials 88 88 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dapr stop printed failure messages but always exited 0, so scripts and CI could not detect failed stops. The same applied to dapr uninstall. Also stop k8s run-file handling no longer proceeds after a template parse error, and no longer falls through into the self-hosted process scan after stopping Kubernetes deployments. Fixes dapr#1667 Signed-off-by: Mukul <nmukul32@gmail.com>
… k8s multi-app stop The stop_without_install e2e encoded the old exit-0-on-failure behavior; it now asserts a non-zero exit and serves as the regression test for it. dapr stop -f -k races with the graceful shutdown performed by the dapr run -f -k process when it receives the stop signal: whichever process deletes the app resources second got NotFound errors from kubectl. Those were previously invisible because stop always exited 0. Pass --ignore-not-found for the stop path's deletes so only real failures surface. Signed-off-by: Mukul <nmukul32@gmail.com>
04724bd to
a369916
Compare
Description
dapr stopanddapr uninstallprint failure messages but always exit 0, so shell scripts and CI pipelines cannot detect that the operation failed.This PR makes the failure paths exit non-zero:
dapr stop --app-id <id>: track per-app stop failures and exit 1 after the loop if any app failed to stop (cmd/stop.go).dapr stop --run-file <f>(self-hosted): exit 1 when stopping via the run file fails.dapr stop --run-file <f> -k: exit 1 on a run-template parse error instead of proceeding intokubernetes.Stopwith a zero-value config, exit 1 when the Kubernetes stop fails, andreturnafterwards instead of falling through into the self-hosted process scan.dapr uninstall: exit 1 when the uninstall returns an error.Repro before this change:
After: exit code is 1 for the same invocation.
Issue reference
Fixes #1667
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
cmd/; happy to add one if maintainers point at a preferred pattern)