-
Notifications
You must be signed in to change notification settings - Fork 743
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
adding omnivore saveurl integration #2237
Conversation
|
b0e8269
to
f703b6e
Compare
return err | ||
} | ||
|
||
_, err = c.wrapped.Do(req) |
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.
There is no error shown in the logs if the API key or the API URL is incorrect. It should at least check the response given by Omnivore API.
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've addressed this by following the pattern in the pocket integration (returning an error for http response codes >=400). The response body is logged as well which might be helpful, but I was unsure if the risk of dumping a ton of text into the log is worth it. Let me know if you'd like to limit or completely omit this part.
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.
When testing, Omnivore seems to return a JSON response like this regardless of the error:
error="omnivore: failed to save URL: status=500 {\"errors\":[{\"message\":\"Unexpected server error\"}]}\n"
It would be nice to parse the JSON response, and log only the error message. It could be done in another pull-request.
f703b6e
to
d1159f6
Compare
d1159f6
to
707f777
Compare
Do you follow the guidelines?
This PR introduces a "save" integration for Omnivore. In order to use it you must generate an API key and input it into the integration section of the settings.
This PR depends on the google/uuid package (which is already an indirect dependency).