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

Improve error message when supervisor unavailable #5465

Merged
merged 2 commits into from Sep 6, 2018
Merged

Conversation

adamhjk
Copy link
Contributor

@adamhjk adamhjk commented Aug 16, 2018

This fixes #5367.

tenor-57499504

Previously, when doing a hab svc (load|unload), or any command that
requires remote control of the supervisor, if there was a network issue,
we would simply return the upstream description of the problem (most
often, something like Connection refused (os error 111).

This wasn't a very user friendly error message. Now, on any SvcClient
command that returns an SvcClientError::Io error, we will print the
following:

✗✗✗
✗✗✗ Unable to contact the Supervisor.
✗✗✗
✗✗✗ If the Supervisor you are contacting is local, this probably means it is not running. You can run a Supervisor in the foreground with:
✗✗✗
✗✗✗ hab sup run
✗✗✗
✗✗✗ Or try restarting the Supervisor through your oerating systems init process.
✗✗✗
✗✗✗ Original error is:
✗✗✗
✗✗✗ Connection refused (os error 111)
✗✗✗

Which is, I think, much more helpful.

Signed-off-by: Adam Jacob adam@chef.io

This fixes #5367.

Previously, when doing a `hab svc (load|unload)`, or any command that
requires remote control of the supervisor, if there was a network issue,
we would simply return the upstream description of the problem (most
often, something like `Connection refused (os error 111)`.

This wasn't a very user friendly error message. Now, on any `SvcClient`
command that returns an `SvcClientError::Io` error, we will print the
following:

```
✗✗✗
✗✗✗ Unable to contact the Supervisor.
✗✗✗
✗✗✗ If the Supervisor you are contacting is local, this probably means it is not running. You can run a Supervisor in the foreground with:
✗✗✗
✗✗✗ hab sup run
✗✗✗
✗✗✗ Or try restarting the Supervisor through your oerating systems init process.
✗✗✗
✗✗✗ Original error is:
✗✗✗
✗✗✗ Connection refused (os error 111)
✗✗✗
```

Which is, I think, much more helpful.

Signed-off-by: Adam Jacob <adam@chef.io>
@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

mwrock
mwrock previously requested changes Aug 16, 2018
Copy link
Contributor

@mwrock mwrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG This will make things SO much better!

"Unable to contact the Supervisor.\n\n\
If the Supervisor you are contacting is local, this probably means it is not running. You can run a Supervisor in the foreground with:\n\n\
hab sup run\n\n\
Or try restarting the Supervisor through your oerating systems init process.\n\n\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oerating -> operating
init process -> init process or Windows service

Windows users will not likely be familiar with the term init process

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

systems -> system's, too

Copy link
Contributor

@christophermaier christophermaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go after some typo fixes.

@christophermaier christophermaier dismissed their stale review August 17, 2018 19:02

Hit the wrong button

@baumanj
Copy link
Contributor

baumanj commented Aug 20, 2018

This is a nice improvement!

I think we could go even one better by checking whether there is a supervisor running locally and/or whether a --remote-sup argument was provided, but this is definitely a good start.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
@baumanj baumanj self-assigned this Sep 6, 2018
@baumanj baumanj dismissed mwrock’s stale review September 6, 2018 17:48

Requested changes made

@baumanj baumanj merged commit 7addefa into master Sep 6, 2018
@baumanj baumanj deleted the adamhjk/5367 branch September 6, 2018 17:49
chef-ci pushed a commit that referenced this pull request Sep 6, 2018
Obvious fix; these changes are the result of automation not creative thinking.
@alex-bender
Copy link
Contributor

Hey guys! It's been a while since I've suggested that improvement and I would say that it is really great to see that changes are accepted. Such a good community, thank you all!

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

Successfully merging this pull request may close these issues.

Error message improvement
6 participants