feat(deploy): Output plain text when Hippo login fails #647
Conversation
Signed-off-by: lidongjies <lidongjies@gmail.com>
There was a problem hiding this comment.
Thanks for tacking this! Unfortunately, we are ideally looking for a bit more than this - what I'm hoping for is to have the top level message reflect the error detail as well as indicating that it's Hippo. That said, I'm not sure what messages the Hippo client returns, and maybe we can't get anything more than "Login failed", in which case fair enough.
src/commands/deploy.rs
Outdated
| @@ -147,7 +147,8 @@ impl DeployCommand { | |||
| self.hippo_username.clone(), | |||
| self.hippo_password.clone(), | |||
| ) | |||
| .await? | |||
| .await | |||
| .context("Problem login Hippo")? | |||
There was a problem hiding this comment.
A couple of things here:
- First, and apologies for nit-picking the English here, but this should be phrased as something like "Problem logging into Hippo"
- Second, it would be nice to extract the reason from the Hippo failure response and present that. What this does is address the second part of Deploy: raw JSON in output if Hippo login fails #614 (say that the error comes from Hippo rather than from Bindle), which is good, but the reason is still buried in the raw JSON. What I'd suggest is that we parse the JSON, look at either the
titleordetailelements (@bacongobbler can probably recommend which) and output that as part of the message. We can still dump the JSON, but this way we get a top-level message which is still precise but is also more readable.
Hope this makes sense, happy to discuss further!
|
Thank you for your detailed reply, which is very helpful to me. I have found the error handling department of the login method in Hippo cli, and I will handle the error information according to your suggestions. |
|
I'd look at HandleLoginFailedException for implementation details as well as what's being returned in this scenario. The error being returned by Hippo doesn't provide much context as to why the login failed, but I'd suggest parsing the title and description. |
Signed-off-by: lidongjies <lidongjies@gmail.com>
|
I have parsed JSON and displayed the 'detailed' field, because neither the 'title' nor the 'detailed' field indicates that the problem occurs in Hippo, so I keep 'problem login into Hippo'. When using an unregistered Hippo username.
When Hippo password is incorrect.
|
There was a problem hiding this comment.
This looks good - thanks for addressing that! There's still one persnickety amendment to the grammar (for which I apologise, but it's user facing), and I also had a suggestion regarding the case where there's no detail. In substance it's spot on though - thanks again!
src/commands/deploy.rs
Outdated
| Ok(token_info) => token_info.token.unwrap_or_default(), | ||
| Err(err) => { | ||
| let error: LoginHippoError = serde_json::from_str(err.to_string().as_str())?; | ||
| bail!("Problem login into Hippo: {}", error.detail); |
There was a problem hiding this comment.
Minor grammar tweak:
| bail!("Problem login into Hippo: {}", error.detail); | |
| bail!("Problem logging into Hippo: {}", error.detail); |
There was a problem hiding this comment.
You note that if the password is incorrect, the detail is blank, so you end up with a trailing colon here (Problem logging into Hippo: ). I'd suggest checking for the empty string and dropping the colon (Problem logging into Hippo) so it looks a bit tidier in this case.
To avoid this making the Err clause so big that it derails the reading flow, you could hive this off into a helper function e.g.
Err(err) => bail!(format_login_error(err)?),
// ...
fn format_login_error(err: &Error) -> anyhow::Result<String> {
let error: LoginHippoError = /* as you have it now */;
if error.detail.is_empty() {
// message without colon
} else {
// message with colon and detail
}
}
There was a problem hiding this comment.
The detail field is not blank, and the trailing colon is part of the detail field.
When using an unregistered Hippo username.
{ "title": "Login failed.", "detail": "Login failed: Account does not exist." }
When Hippo password is incorrect.
{ "title": "Login failed.", "detail": "Login failed:" }
There was a problem hiding this comment.
fn format_login_error(err: &anyhow::Error) -> anyhow::Result<String> {
let error: LoginHippoError = serde_json::from_str(err.to_string().as_str())?;
if error.detail.ends_with(": ") {
Ok(format!(
"Problem logging into Hippo: {}",
error.detail.replace(": ", ".")
))
} else {
Ok(format!("Problem logging into Hippo: {}", error.detail))
}
}When using an unregistered Hippo username.
Error: Problem login into Hippo: Login failed: Account does not exist.
When Hippo password is incorrect.
Error: Problem login into Hippo: Login failed.
There was a problem hiding this comment.
Oops, sorry, my misunderstanding - we should tweak this on the Hippo side. I like your solution though!
There was a problem hiding this comment.
(that is to say.... we should do what you suggest even if it's only needed temporarily)
Cargo.toml
Outdated
| @@ -48,6 +49,7 @@ url = "2.2.2" | |||
| wasi-outbound-http = { path = "crates/outbound-http" } | |||
| wasmtime = "0.35.3" | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Super pedantic: I don't think the extra line is needed?
There was a problem hiding this comment.
👌 Of course, I'll delete this blank line
Signed-off-by: lidongjies <lidongjies@gmail.com>
Signed-off-by: lidongjies lidongjies@gmail.com
With this PR, if it fails to obtain the Hippo token when executing the "spin deploy" command, the following error message will be output
Related issue #614