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

CLI wrongly returns exit code 0 when providing not existing wallet id #397

Closed
piotr-iohk opened this issue Jun 11, 2019 · 4 comments
Closed
Assignees

Comments

@piotr-iohk
Copy link
Contributor

piotr-iohk commented Jun 11, 2019

Release Operating System Cause
next Linux Code

Context

cf -> #395 (comment)
When providing wallet id that does not exist on the node (e.g. which was previously deleted) the API returns 404 and appropriate message I couldn't find a wallet with the given id: <walletId>. Although CLI returns the message correctlly, the exit code in such a case is 0 which is somewhat inconsistent. The exit code should be 1.
There are few integration tests that illustrate current behavior:

Steps to Reproduce

Run one of the commands

cardano-wallet wallet get 1111111111111111111111111111111111111111
cardano-wallet wallet delete 1111111111111111111111111111111111111111
cardano-wallet wallet update 1111111111111111111111111111111111111111 --name "asdf"
cardano-wallet transaction create 1111111111111111111111111111111111111111 --payment 22@2cWKMJemoBahmFD6eqM76PqSVxxbobW4QbnF2ApBbR1c7JcrEhyq8S8YSrweZJPGGBcoY

And then

echo $?

Expected behavior

Output of the echo command should be 1

Actual behavior

Output is 0


Resolution Plan

PR

Number Base
#402 develop
#399 develop

QA

In #399 I have also modified corresponding integration tests to expect for response code 1 in case the response from the server is faulty. It turned out that the fix also addressed several other cases not mentioned in original bug description:

  • not enough utxo when posting tx
  • not enough fee when posting tx
  • not enough money when posting tx
  • wrong password when posting tx
@jonathanknowles
Copy link
Member

Fix available: #402

@jonathanknowles
Copy link
Member

@piotr-iohk My apologies. I hadn't realized that you had already created a PR to address this (PR #399). Feel free to disregard my PR #402 if necessary.

@piotr-iohk
Copy link
Contributor Author

Woops, my bad for not updating the bug... ¯_(ツ)_/¯ (@jonathanknowles - essentially I made the same change as you, plus I've updated integration tests. I suppose you did not do the latter, hence #402 fails on CI)

@KtorZ
Copy link
Member

KtorZ commented Jun 14, 2019

LGTM 👍

@KtorZ KtorZ closed this as completed Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants