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

Missing exit status when command errors #174

Closed
victorb opened this issue Oct 12, 2017 · 4 comments
Closed

Missing exit status when command errors #174

victorb opened this issue Oct 12, 2017 · 4 comments
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/ready Ready to be worked

Comments

@victorb
Copy link

victorb commented Oct 12, 2017

root@cluster-0:~# ipfs-cluster-ctl peers add $(cat /tmp/ipfs-cluster-server-addr)
An error ocurred:
  Code: 500
  Message: no connections to: QmNj4zW8SidPEU4amfAdgtaKjG3c2GffmzWWvCMeSYuRqp

root@cluster-0:~# echo $?
0

Exit code should have been something else than 0

@hsanjuan hsanjuan added kind/bug A bug in existing code (including security flaws) exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up status/ready Ready to be worked labels Oct 12, 2017
@hsanjuan hsanjuan added this to the User feedback and bugs [Q4O3] milestone Oct 12, 2017
@dgrisham
Copy link
Collaborator

dgrisham commented Oct 12, 2017

@victorbjelkholm I noticed this while working on #147, once that gets merged this should be fixed (specifically by this).

@victorb
Copy link
Author

victorb commented Oct 12, 2017

Doesn't fmt.Errorf just prints the error rather than setting exit code? Feels like os.Exit(nonZero) has to be called somewhere if the exit code is to be set correctly.

https://github.com/ipfs/ipfs-cluster/pull/147/files#diff-26b121c3d48b28e2155589e577d025f5R478

@hsanjuan
Copy link
Collaborator

@victorbjelkholm checkErr does that.

But actually, now that you mention, ipfs-cluster-ctl returns 0 in these cases because the request was successful (it was performed correctly, reached it's destination, and a well-formatted answer came back). Therefore cluster-ctl did its job. The error is from cluster not from the client. If you set json output the error will be in json, so your application can parse it. cluster-ctl returns non 0 exit status when something went bad and could not perform the request, in such case, output might not be parseable etc.

This is weird, but consistent with having a very clear line between what's cluster and what's a cluster api client.

That said, I don't like weird things, should probably be changed. @dgrisham your fix prevents the actual error response from being formatted to screen btw...

@dgrisham
Copy link
Collaborator

@victorbjelkholm this should be fixed with #147, you'll get exit code 2 if an HTTP error code (>= 400) is received from the API call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

3 participants