-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Return 404 Not Found for failed path resolutions #3777
Conversation
return | ||
} | ||
fallthrough | ||
default: | ||
// all other erros | ||
webError(w, "Path Resolve error", err, http.StatusBadRequest) | ||
webError(w, "ipfs cat "+urlPath, err, http.StatusNotFound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should mention error here. HTTP codes are hidden in browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The printed message contains err
, so it's gonna be something like: "ipfs cat /ipns/foo.com: Could not resolve name"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I have missed that.
@lgierth So running |
Note: I'll amend this to return a different error "Code" in the response, instead of hijacking the http status code. |
License: MIT Signed-off-by: Lars Gierth <[email protected]>
bf53b2a
to
38504d1
Compare
License: MIT Signed-off-by: Lars Gierth <[email protected]>
38504d1
to
807ffb9
Compare
done |
Checking why CI is upset |
Travis and Circle are happy now -- I didn't bother restarting the second Travis build though. |
Wait this isn't quite right |
Nevermind, I tested with the wrong binary. It does correctly return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, Thanks @lgierth :)
Return 404 Not Found for failed path resolutions This commit was moved from ipfs/kubo@2cc5ce4
Fixes #2286 -- bonus: also kills gateway's namesys dependency
But: I just noticed that the CLI assumes "command not found" for 404 responses. Great.