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

Make "unknown error" client errors a bit more informative #323

Conversation

itowlson
Copy link
Contributor

The current "Unknown error" is very hard to diagnose because it depends on the server providing everything you need in the message. This is not a good thing to rely on, especially for unfortunate server devs trying to get their stuff working with the existing client, so this PR adds the following information to the "unknown error" message:

  • The URL that produced the unexpected response
  • The operation being carried out on that URL
  • The status code that was returned

A minor change is that it will now say Unknown error: (no error message in response) rather than a trailing colon and space Unknown error: . A complete message now looks like Unknown error response: Create to http://localhost:5000/v1/_i/weather/1.2.3-ivan-20220317112555054@a4d030d845438e530f12cbf85422e82a65327443786e0c9f59fbbf282adde7e3 returned status 201 Created: (no error message in response)

@itowlson
Copy link
Contributor Author

@thomastaylor312 do you own the GH actions for this? I am getting a failure on something to do with large regexes, but my PR does not change anything involving large regexes.

@thomastaylor312
Copy link
Contributor

@itowlson Yeah, this is due to a new RUSTSEC vulnerability. It shouldn't effect us but running a cargo update should solve the problem

Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

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

This is much better! Feel free to merge if you don't want to do the cargo update and I'll handle it later

@itowlson itowlson merged commit e6b6851 into deislabs:main Mar 17, 2022
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.

3 participants