Skip to content
This repository was archived by the owner on Feb 11, 2025. It is now read-only.

Comments

improve error messages for network connection failures#232

Merged
technosophos merged 2 commits intodeislabs:mainfrom
technosophos:fix/231-make-better-mistakes
Sep 29, 2021
Merged

improve error messages for network connection failures#232
technosophos merged 2 commits intodeislabs:mainfrom
technosophos:fix/231-make-better-mistakes

Conversation

@technosophos
Copy link
Contributor

This traps networking errors and makes a noble effort to be informative.

Most, if not all, network errors will be due to either network failure or pointing at the wrong URL. This PR tries to catch those that result in hitting the wrong URL.

Server responds 400 BAD REQUEST:

$ bindle --server http://bindle.f1sh.ca:8080/v1 search
Server has encountered an error: The request could not be handled by the server. Verify your Bindle server URL

A networking error occurred during connection (e.g. wrong port, server disconnects abruptly):

$ bindle --server http://bindle.f1sh.ca/v1 search
Server has encountered an error: Protocol error. Verify the Bindle URL

Closes #231

Signed-off-by: Matt Butcher matt.butcher@microsoft.com

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
@technosophos technosophos self-assigned this Sep 10, 2021
@technosophos technosophos added the bug Something isn't working label Sep 10, 2021
},
/// A server error was encountered. Contains an optional message from the server
#[error("Server has encountered an error: {0:?}")]
#[error("Server has encountered an error: {}", .0.clone().unwrap_or_else(||"Protocol error. Verify the Bindle URL".to_owned()))]
Copy link
Contributor

Choose a reason for hiding this comment

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

From what you said this could result from the wrong port or from a network disconnect? If so I feel like "Server has encountered an error" may send people down the wrong path here... but also very hard to get clear wording for what could be a wide range of cases that are hard to tell apart!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe "Networking error: {}" would be more accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! "Error contacting server"

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
@thomastaylor312
Copy link
Contributor

@technosophos Did you need any more feedback here or can we merge?

@technosophos
Copy link
Contributor Author

Merge away. I forgot about this PR

@technosophos technosophos merged commit 6a66116 into deislabs:main Sep 29, 2021
@technosophos technosophos deleted the fix/231-make-better-mistakes branch September 29, 2021 22:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Time for another error message pass

3 participants