Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exit with error for run command #684

Closed
mukundansundar opened this issue Apr 9, 2021 · 6 comments · Fixed by #905
Closed

Fix exit with error for run command #684

mukundansundar opened this issue Apr 9, 2021 · 6 comments · Fixed by #905
Assignees
Labels
kind/bug Something isn't working triaged/resolved The issue has been triaged
Milestone

Comments

@mukundansundar
Copy link
Collaborator

Expected Behavior

On error, run command exits with error

Actual Behavior

Without installing dapr, running dapr run fails as expected. But the exit value is 0 instead of expected non-zero val.

$ dapr run test
❌  stat ~/.dapr/components: no such file or directory
$ echo $?
0

Additional error scenarios also need to be investigated and fixed if needed.

Steps to Reproduce the Problem

Don't install dapr, and then run dapr run test.

Release Note

RELEASE NOTE: RESOLVED On error, run exits with an error code

@dapr-bot
Copy link
Collaborator

dapr-bot commented Jan 4, 2022

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged (pinned, good first issue, help wanted or triaged/resolved) or other activity occurs. Thank you for your contributions.

@shubham1172
Copy link
Member

@mukundansundar, JS-SDK is facing issues while testing due to this behavior. While we have a workaround at the moment, it will be nice to have an elegant fix. Can I take this issue up?

One thing that we were suspecting offline was that it can break existing consumers of the CLI. @XavierGeerinck suggested that this behavior can be kept behind a flag, for example --error-out-on-fail, and then announce a deprecation date/make that as default behavior. This way we will also avoid breaking existing SDK test runners which might be relying on this. Thoughts?

@artursouza
Copy link
Member

@mukundansundar, JS-SDK is facing issues while testing due to this behavior. While we have a workaround at the moment, it will be nice to have an elegant fix. Can I take this issue up?

One thing that we were suspecting offline was that it can break existing consumers of the CLI. @XavierGeerinck suggested that this behavior can be kept behind a flag, for example --error-out-on-fail, and then announce a deprecation date/make that as default behavior. This way we will also avoid breaking existing SDK test runners which might be relying on this. Thoughts?

In this case, this is a bug, so there is no need for a flag. Clients that depend on a bug will naturally break, which is OK here.

@yaron2
Copy link
Member

yaron2 commented Feb 25, 2022

@mukundansundar, JS-SDK is facing issues while testing due to this behavior. While we have a workaround at the moment, it will be nice to have an elegant fix. Can I take this issue up?
One thing that we were suspecting offline was that it can break existing consumers of the CLI. @XavierGeerinck suggested that this behavior can be kept behind a flag, for example --error-out-on-fail, and then announce a deprecation date/make that as default behavior. This way we will also avoid breaking existing SDK test runners which might be relying on this. Thoughts?

In this case, this is a bug, so there is no need for a flag. Clients that depend on a bug will naturally break, which is OK here.

Agree

@shubham1172
Copy link
Member

Clients that depend on a bug will naturally break, which is OK here.

Sounds good, thanks!

@shubham1172
Copy link
Member

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working triaged/resolved The issue has been triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants