Make CLI exit with error code if dapr run or invoke fails #905
Conversation
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
Codecov Report
@@ Coverage Diff @@
## master #905 +/- ##
==========================================
+ Coverage 22.89% 22.98% +0.09%
==========================================
Files 29 29
Lines 1612 1614 +2
==========================================
+ Hits 369 371 +2
Misses 1193 1193
Partials 50 50
Continue to review full report at Codecov.
|
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
|
@shubham1172 if you are modifying the run command exit flow ... please make sure that all processes and child processes that have been spun are closed on exit. |
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
|
@mukundansundar I have added
No premature exits have been added in place. |
shubham1172
changed the title
|
@shubham1172 I think we can add exit here also - Line 86 in 34353a2 |
|
@pravinpushkar nice, let me add it too. Thanks for the suggestion! |
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
|
Added the changes. /cc @mukundansundar @pravinpushkar can you please review it? |
shubham1172
changed the title
| if output.AppErr != nil { | ||
| exitWithError = true | ||
| print.FailureStatusEvent(os.Stderr, fmt.Sprintf("Error exiting App: %s", output.AppErr)) | ||
| } else if output.AppCMD != nil && (output.AppCMD.ProcessState == nil || !output.AppCMD.ProcessState.Exited()) { | ||
| err = output.AppCMD.Process.Kill() | ||
| if err != nil { | ||
| print.FailureStatusEvent(os.Stderr, fmt.Sprintf("Error exiting App: %s", err)) |
There was a problem hiding this comment.
@shubham1172 If it errors out here and line number 316 then exitWithError will not be set. Is this expected ? or it is not possible to have this behavior. Just wanted to bring it your notice.
There was a problem hiding this comment.
Yes @pravinpushkar, that was an existing behavior that I haven't changed with this PR. Now that I think more about it I think we should exit with error in that scenario too. What do you think?
cc @mukundansundar
There was a problem hiding this comment.
Have added the same to this PR!
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
…ubham1172/cli into shubham1172/cli-exit-code-fix
| @@ -83,7 +83,7 @@ dapr invoke --unix-domain-socket --app-id target --method sample --verb GET | |||
| if err != nil { | |||
| err = fmt.Errorf("error invoking app %s: %s", invokeAppID, err) | |||
| print.FailureStatusEvent(os.Stderr, err.Error()) | |||
| return | |||
| os.Exit(1) | |||
There was a problem hiding this comment.
@shubham1172 Can you add an e2e for this scenario?
There was a problem hiding this comment.
@mukundansundar added the test.
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
Signed-off-by: Shubham Sharma shubhash@microsoft.com
Description
This PR looks out for errors from Dapr command and app command, and bubbles them to the Dapr CLI.
It also adds error code for
dapr invokefailures.Scenario 1
Taken from the original issue, run Dapr without installing it.
Before
Now
Scenario 2
Run an app that exits with a error code.
Say an example script:
Before
CLI logs
Now
CLI logs
Issue reference
Please reference the issue this PR will close: #684
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: