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

feat: RoverClientError::CouldNotConnect #518

Merged
merged 3 commits into from
May 7, 2021
Merged

Conversation

EverlastingBugstopper
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper commented May 6, 2021

fixes #489

before:

$ rover subgraph introspect http://localhost:4000
error: encountered an error while sending a request
        This error was unexpected! Please submit an issue with any relevant details about what you were trying to do: https://github.com/apollographql/rover/issues/new

after:

$ rover subgraph introspect http://localhost:4000
error: Could not connect to http://localhost:4000/.
        Make sure the endpoint is spelled correctly and accepting connections.

other HTTP request related errors now just use the default reqwest formatting, and provides no suggestion. we can incrementally improve this as time goes on.


couple fun new patterns here for intelligent error handling in this PR. This error currently applies to any request made by rover-client, not just introspection. folks might see this error if Studio ever starts refusing incoming TCP connections. Most folks will run into this when running introspection on an invalid endpoint. I'm wondering if this catch-all approach is good here (returning this error type on every request made by rover-client where the server refuses a connection) or if we should try to provide more granular info based on the command they're running or if the Url it can't connect to is supplied by the user vs. one that we coded.

@EverlastingBugstopper EverlastingBugstopper added the feature 🎉 new commands, flags, functionality, and improved error messages label May 6, 2021
@EverlastingBugstopper EverlastingBugstopper added this to the May 11 - GA milestone May 6, 2021
Comment on lines +78 to +82
if let Some(url) = .url {
url.to_string()
} else {
"Unknown URL".to_string()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty friggin neat if i do say so myself

Copy link
Member

Choose a reason for hiding this comment

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

this is neat!

/// Encountered an error sending the request.
#[error("encountered an error while sending a request")]
#[error(transparent)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i changed this to transparent so that it would just print out the whole error message from reqwest. they're a bit verbose but they at least give some info to the user that we were obfuscating previously. i imagine as we see these crop up we can add more of our own error types

@@ -115,6 +116,7 @@ impl Display for Suggestion {
)
}
Suggestion::Adhoc(msg) => msg.to_string(),
Suggestion::CheckServerConnection => "Make sure the endpoint is accepting connections.".to_string()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't love this wording. if anybody has suggestions 😉 lemme know

@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/better-introspect-error branch 2 times, most recently from 89a9475 to 8b0bea0 Compare May 6, 2021 21:17
src/error/metadata/suggestion.rs Outdated Show resolved Hide resolved
Comment on lines +78 to +82
if let Some(url) = .url {
url.to_string()
} else {
"Unknown URL".to_string()
}
Copy link
Member

Choose a reason for hiding this comment

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

this is neat!

@lrlna lrlna merged commit e7d8617 into main May 7, 2021
@lrlna lrlna deleted the avery/better-introspect-error branch May 7, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

better error for introspection unreachable endpoint
2 participants